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

[Thomas Lamprecht]

On 6/4/18 12:28 PM, Stoiko Ivanov wrote:
> Thanks for the great feedback! - agree with most things and went ahead
> and improved them.
> Further comments inline:
> On Fri, 1 Jun 2018 16:04:46 +0200
> Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:
> 
>> On 5/29/18 4:30 PM, Stoiko Ivanov wrote:
>> ..snip.. 
>>
>> 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.
> 
> not quite sure how the class usage would help directly here (the method
> gets invoked from PVE::CLI:pveum (we could call it explicitly with the
> needed class (e.g. PVE::API2::User), but would still need to provide
> the method-name to get the return values. - but probably I still don't
> have the complete picture.
> 
> It probably would be the cleanest (from a not duplicating code point of
> view) to add another parameter to 'cmddef', which signals
> handle_cmd/handle_simple_cmd, whether or not to pass the return
> information to the provided output-sub - Would that sound reasonable?
> 

You do not need to expand $cmddef, you have everything needed already.

Instead of getting the info and pass it to $outsub in 1/2 you could just
pass the $method_name, which you have available in the scope $outsub gets
called, then you could just get the info needed there with
$class->map_method_by_name($name)

Oh, and maybe we could omit $formatopts and reuse the returns property from
the schema for this purpose? 
We could encode default values there, define if certain values should be
hidden (or optionally viewable) and so on?
You already do this somewhat with $returnformats, but I would not use it as
fallback but as prime source for all info.
This would need to add return schema info on a few places, but that is
a good thing to have now already.


On another node, please move the print_text_table and print_entry to the
other print_* methods in the CLIHandler class.

> 
>> ..snip.. 
>>> +    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)
> sounds reasonable/cleaner.
> 

a rough (untested) outline of the sub could look like:

sub print_text_table {
    my ($class, $name, $res_data) = @_;

    my $info = $class->map_method_by_name($name);

    # TODO: we've different return types
    my $fmt = $info->{"returns"}->{"items"}->{"properties"};

    my @columns = grep { !$_->{hidden} } @$fmt;
    my $last_col = @columns[-1];

    # foreach ...
}

(just a suggestion, if you find clearer/better ways, great!)

Oh, and Alwin also worked on table formatting, maybe it could be useful
to have a talk with him.

cheers,
Thomas