From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: "Dominic Jäger" <d.jaeger@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part
Date: Tue, 17 Nov 2020 09:48:26 +0100 [thread overview]
Message-ID: <5dc95bdf-1ead-f32b-bc28-aeddb9d643fa@proxmox.com> (raw)
In-Reply-To: <20201117083556.GA1845190@mala.proxmox.com>
On 17.11.20 09:35, Dominic Jäger wrote:
> On Mon, Nov 16, 2020 at 06:12:37PM +0100, Thomas Lamprecht wrote:
>>
>> Or move out even more than just the format string generation out, so that it
>> becomes a simple loop calling
>>
>> $text .= render_task_plain($vmid, $task);
>>
>> or something similar to that.
>
> my $namelength = 20;
> $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n",
> qw(VMID NAME STATUS TIME SIZE FILENAME));
> my $render_task_plain = sub {
> my ($vmid, $task) = @_;
> my $successful = $task->{state} eq 'ok';
> $text .= sprintf("%-10s %-${namelength}s %-6s %10s " .
> ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid},
> $name, $task->{state}, format_time($task->{backuptime}), $size,
> $filename);
> };
> foreach my $task (@$tasklist) {
> $text .= render_task_plain($vmid, $task);
> }
>
> Not sure about this, we cannot move the heading into render_task_plain => Still
> two format strings? So I don't really see how we would benefit from the
> additional sub.
the line rendering is cleanly separated, headings are headings, those are
often separated - one would need a column definition structure to solve that,
e.g.:
my $columns = {
'heading 1' => "%fmt1",
'heading 2' => "%fmt2",
};
maybe reusing the exisitng CLI formatter module (with borders disabled) from
pve-common could be used? So that we do not have two slightly over engineered
ways of doing this ^^
But, I'd go for a middle ground for now (see below), as that could be another
wormhole to get pulled into. :)
>
>> we could avoid the sub and the if by using the $size_conversion directly in
>> the format string?
>
> I personally would prefer this idea. Then we can still decide if we prefer
> 1. Short, but two format strings written out
>
> my $namelength = 20;
> $text .= sprintf ("%-10s %-${namelength}s %-6s %10s %10s %s\n",
> qw(VMID NAME STATUS TIME SIZE FILENAME));
> foreach my $task (@$tasklist) {
> my $successful = $task->{state} eq 'ok';
> $text .= sprintf("%-10s %-${namelength}s %-6s %10s " .
> ($successful ? "%10s": "%8.2fMB")." %s\n", $task->{vmid}, $name,
> $task->{state}, format_time($task->{backuptime}), $size, $filename);
If, I'd prefer some other formatting, each param on it's own line:
my $size_fmt = $successful ? "%10s": "%8.2fMB";
$text .= sprintf(
"%-10s %-${namelength}s %-6s %10s $size_fmt %s\n",
$task->{vmid},
$name,
$task->{state},
format_time($task->{backuptime}),
$size,
$filename,
);
above format, oriented on what rustfmt normally does for such things, is IMO
more readable than the other proposed variants.
> }
>
> 2. or a longer version. We could put all the decisions into the $fmt sub (idea
> thanks to Hannes). Then it's a little longer, but relatively easy to read I
> think, and has no two written out format strings.
>
> my $namelength = 20;
> my $fmt = sub {
> my ($successful) = @_;
> my $fmt = "%-10s %-${namelength}s %-6s %10s ";
> $fmt .= $successful ? "%10s": "%8.2fMB";
> $fmt .= " %s\n";
> return $fmt;
> };
> $text .= sprintf ($fmt->(1), qw(VMID NAME STATUS TIME SIZE FILENAME));
> foreach my $task (@$tasklist) {
> my $name = substr($task->{hostname}, 0, $namelength);
> my $successful = $task->{state} eq 'ok';
> my $size = $successful ? format_size ($task->{size}) : 0;
> my $filename = $successful ? $task->{target} : '-';
> $text .= sprintf($fmt->($successful), $task->{vmid}, $name,
> $task->{state}, format_time($task->{backuptime}), $size, $filename);
> }
>
prev parent reply other threads:[~2020-11-17 8:48 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-16 10:14 Dominic Jäger
2020-11-16 17:12 ` Thomas Lamprecht
2020-11-17 8:35 ` Dominic Jäger
2020-11-17 8:48 ` Thomas Lamprecht [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5dc95bdf-1ead-f32b-bc28-aeddb9d643fa@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=d.jaeger@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal