[pve-devel] [PATCH common 2/2] add print_text_table, print_entry to CLIHandler

[Thomas Lamprecht]

On 5/29/18 4:30 PM, Stoiko Ivanov wrote:
> These two function could serve as a generic output sub for various CLI
> utilities
>  * print_text_table can be configured get a list of items and print them in a
>    tabular fashion, the column headers can be optionally provided as argument,
>    defaulting to the title attribute or name of the displayed property
>    (as defined in the regsiter_method call). An optional cutoff argument limits
>    the length of the printed data

s/regsiter_method/register_method/

> 
>  * print_entry prints out a single entry, handling array-refs as properties
>  * both are needed for the patch sent for pveum (pve-access-control), and could
>    be used in more contexts in the future
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  src/PVE/CLIHandler.pm | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index 58e7a05..512926c 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -563,4 +563,50 @@ sub run_cli_handler {
>      exit 0;
>  }
>  

In general: please use some new lines to improve readability.
It's fine, and often even wanted, that a few lines are stuck together if
they built upon each other or can be logically grouped together,
but this is just to much for me to handle.

> +# used to print the result of listing - expects the API to return an array
> +# and to have the results key of the API call defined.
> +sub print_text_table {
> +    my ($class, $formatopts, $data, $info) = @_;

class is not used anywhere, so why making this a class method and not a
static helper?

What about splitting out the formatting logic stuff in it's own (maybe
"private") sub and let this class get the info itself through
map_method_by_name? It's just a hash look to the ifno ref, so IMO no
need for patch 1/2. Just as an idea, then the $class usage here would
actually be useful.

> +    my $returnformats = $info->{"returns"}->{"items"}->{"properties"};

could be the callers responsibility to give me the return properties?
(see above note for splitting this up in static and class method)

> +    my ($formatstring, @keylist, @titles, %defaults, %cutoffs, $last_col);
> +    @keylist = map { $_->{'key'} } @$formatopts;

we normally do not use quotes for hard coded hash key accesses, if not
needed (i.e., no spaces or minus (etc) symbols used).

> +    $last_col = $$formatopts[$#{$formatopts}];
> +    foreach my $col ( @$formatopts ) { > +	my ($key, $title, $cutoff) = @$col{ qw(key title cutoff) };

I'd rather remove $title from above list declaration...

> +	$title = $returnformats->{$key}->{'title'} // $key if !defined $title;

... and use:
     my $title = $col->{title} // $returnformats->{$key}->{'title'} // $key;
it makes it clearer what the default value chain is with only reading on 
line of code, then I'd directly do:
     push @titles, ($title);

> +	$defaults{$key} = $col->{'default'} // $returnformats->{$key}->{'default'};
> +	my $titlelen = length $title;
> +	push @titles, ($title);
> +	my $longest = $titlelen;

in general: what the code does looks actually quite OK (I need to dig
still a bit deeper, but before I invest that time I'd rather clarify if
the used approach is OK.)

> +	foreach my $entry (@$data) {
> +	    my $len = (length $entry->{$key}) // 0;
> +	    $longest = $len if $len > $longest;
> +	}
> +	$longest = $cutoff if (defined $cutoff && $cutoff < $longest);

> +	my $printalign = $longest > $titlelen ? '-' : '';
> +	if ($col == $last_col) {
> +	    $formatstring .= "%$printalign$titlelen"."s\n";
> +	} else {
> +	    $formatstring .= "%$printalign$longest".'s ';
> +	}
> +	$cutoffs{$key} = $longest;
> +    }
> +    printf $formatstring, @titles;
> +    foreach my $line (sort { $a->{$keylist[0]} cmp $b->{$keylist[0]} } @$data){
> +        printf $formatstring, map { substr(($line->{$_} // $defaults{$_}),0 , $cutoffs{$_}) } @keylist;
> +    }
> +}
> +
> +sub print_entry {
> +    my ($class, $entry) = @_;
> +    foreach my $item (sort keys %$entry) {
> +	if (ref($entry->{$item}) eq 'ARRAY'){
> +	    printf "%s: [ %s ]\n", $item, join(", ", @{$entry->{$item}});
> +	} else {
> +	    printf "%s: %s\n", $item, $entry->{$item};
> +	}
> +    }
> +}
> +
> +
>  1;
>