* [pve-devel] [PATCH manager] vzdump mail: Refactor text part
@ 2020-11-16 10:14 Dominic Jäger
2020-11-16 17:12 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Dominic Jäger @ 2020-11-16 10:14 UTC (permalink / raw)
To: pve-devel
Less lines, less nesting, less duplicate code, less magic numbers.
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
Depends on the previous patch for #3136.
Admittedly, using a sub and ternary operator might be seen as more complex than
just the if-statements. So I'm not mad if we leave it as it is.
But comparing the (first) parameters of 3 sprintf is not that much fun either.
PVE/VZDump.pm | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 1096d594..aaf8a84d 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -258,24 +258,17 @@ sub sendmail {
# text part
my $text = $err ? "$err\n\n" : '';
- $text .= sprintf ("%-10s %-20s %-6s %10s %10s %s\n", qw(VMID NAME STATUS TIME SIZE FILENAME));
+ my $namelength = 20;
+ my $conversions = sub { "%-10s %-${namelength}s %-6s %10s $_[0] %s\n" };
+ $text .= sprintf ($conversions->("%10s"), qw(VMID NAME STATUS TIME SIZE FILENAME));
foreach my $task (@$tasklist) {
- my $vmid = $task->{vmid};
- if ($task->{state} eq 'ok') {
-
- $text .= sprintf ("%-10s %-20s %-6s %10s %10s %s\n", $vmid,
- substr($task->{hostname}, 0, 20),
- $task->{state},
- format_time($task->{backuptime}),
- format_size ($task->{size}),
- $task->{target});
- } else {
- $text .= sprintf ("%-10s %-20s %-6s %10s %8.2fMB %s\n", $vmid,
- substr($task->{hostname}, 0, 20),
- $task->{state},
- format_time($task->{backuptime}),
- 0, '-');
- }
+ 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} : '-';
+ my $size_conversion = $successful ? "%10s": "%8.2fMB";
+ $text .= sprintf($conversions->($size_conversion), $task->{vmid}, $name,
+ $task->{state}, format_time($task->{backuptime}), $size, $filename);
}
my $text_log_part;
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part
2020-11-16 10:14 [pve-devel] [PATCH manager] vzdump mail: Refactor text part Dominic Jäger
@ 2020-11-16 17:12 ` Thomas Lamprecht
2020-11-17 8:35 ` Dominic Jäger
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2020-11-16 17:12 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominic Jäger
On 16.11.20 11:14, Dominic Jäger wrote:
> Less lines, less nesting, less duplicate code, less magic numbers.
>
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> Depends on the previous patch for #3136.
>
> Admittedly, using a sub and ternary operator might be seen as more complex than
> just the if-statements. So I'm not mad if we leave it as it is.
we could avoid the sub and the if by using the $size_conversion directly in
the format string?
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.
> But comparing the (first) parameters of 3 sprintf is not that much fun either.
>
> PVE/VZDump.pm | 27 ++++++++++-----------------
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 1096d594..aaf8a84d 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -258,24 +258,17 @@ sub sendmail {
>
> # text part
> my $text = $err ? "$err\n\n" : '';
> - $text .= sprintf ("%-10s %-20s %-6s %10s %10s %s\n", qw(VMID NAME STATUS TIME SIZE FILENAME));
> + my $namelength = 20;
> + my $conversions = sub { "%-10s %-${namelength}s %-6s %10s $_[0] %s\n" };
if, I'd call this $fmt but actually I'd rather avoid the sub completely (see above)
> + $text .= sprintf ($conversions->("%10s"), qw(VMID NAME STATUS TIME SIZE FILENAME));
> foreach my $task (@$tasklist) {
> - my $vmid = $task->{vmid};
> - if ($task->{state} eq 'ok') {
> -
> - $text .= sprintf ("%-10s %-20s %-6s %10s %10s %s\n", $vmid,
> - substr($task->{hostname}, 0, 20),
> - $task->{state},
> - format_time($task->{backuptime}),
> - format_size ($task->{size}),
> - $task->{target});
> - } else {
> - $text .= sprintf ("%-10s %-20s %-6s %10s %8.2fMB %s\n", $vmid,
> - substr($task->{hostname}, 0, 20),
> - $task->{state},
> - format_time($task->{backuptime}),
> - 0, '-');
> - }
> + 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} : '-';
> + my $size_conversion = $successful ? "%10s": "%8.2fMB";
> + $text .= sprintf($conversions->($size_conversion), $task->{vmid}, $name,
> + $task->{state}, format_time($task->{backuptime}), $size, $filename);
> }
>
> my $text_log_part;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part
2020-11-16 17:12 ` Thomas Lamprecht
@ 2020-11-17 8:35 ` Dominic Jäger
2020-11-17 8:48 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Dominic Jäger @ 2020-11-17 8:35 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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.
> 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);
}
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);
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part
2020-11-17 8:35 ` Dominic Jäger
@ 2020-11-17 8:48 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2020-11-17 8:48 UTC (permalink / raw)
To: Dominic Jäger; +Cc: Proxmox VE development discussion
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);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-17 8:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 10:14 [pve-devel] [PATCH manager] vzdump mail: Refactor text part 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 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