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

[Stoiko Ivanov]

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?


>..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.

Regards,
stoiko