Re: [pve-devel] [PATCH manager] vzdump mail: Refactor text part

[Thomas Lamprecht <t.lamprecht@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);
> }
>