some initial comments inline
On Wed, Oct 11, 2017 at 06:58:05PM +0200, Philip Abernethy wrote:
> instead of 5 slightly different calls to RESTHandler::usage_str this
> introduces a wrapper function that handles all required cases and is
> capable of resolving sub-commands and aliases.
> Adds a subroutine to print the short help for a command in case no
> subcommand was given.
> Modifies handle_cmd and print_bash_completion to allow for parsing of
> subcommands and aliases.
> ---
> src/PVE/CLIHandler.pm | 293
> ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 215 insertions(+), 78 deletions(-)
>
> diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
> index e61fa6a..683403b 100644
> --- a/src/PVE/CLIHandler.pm
> +++ b/src/PVE/CLIHandler.pm
> @@ -2,7 +2,6 @@ package PVE::CLIHandler;
>
> use strict;
> use warnings;
> -use Data::Dumper;
Please put that in one of the cleanup patches.
>
> use PVE::SafeSyslog;
> use PVE::Exception qw(raise raise_param_exc);
> @@ -48,6 +47,85 @@ my $complete_command_names = sub {
> return $res;
> };
>
> +my $generate_usage_str;
> +$generate_usage_str = sub {
> + my ($args) = @_;
> + die "not initialized" if !($cmddef && $exename && $cli_handler_class);
> + die 'format required' if !$args->{format};
If format is not optional and everything else is, it would make more
sense to make this a sub($%) (iow. 1 fixed parameter and an inline-hash,
rather than a hash-reference).
=>
my ($format, %args) = @_;
In fact, the function seems way to littered with $args-> accesses, in
part also due to the fact that you fill $args with more than just
parameters (pwcallback, stringfilemap):
my ($format, $cmd, $indent, $sect_sep, $sortfunc, $base, $prefix) = @_;
Isn't all too long and will make the code much more readable. $prefix is only
used internally for the recursion, so it goes last here.
You can of course use the first version, too, but then please extract
the parameters into variables.
Later on you also add `pwcallback` and `stringfilemap` to $args, both
of which come from $cli_handler_class which does not change during the
recursion anyway, so they should just be queried where they're actually
used.
> +
> + # Set the defaults
> + $args->{sortfunc} //= sub {
> + my ($hash) = @_;
> + return sort keys %$hash;
> + };
> + $args->{cmd} = $cmddef->{$args->{cmd}}->{alias}
> + if (defined($args->{cmd}) && ref($cmddef->{$args->{cmd}}) eq 'HASH' &&
> defined($cmddef->{$args->{cmd}}->{alias}));
Please use more temporary variables for this and other hash accesses as
well. We already have enough code lines consisting of 90% hash accesses
sometimes even to the same variable over and over.
Something like:
my $cmd = $args->{cmd};
my $cmdinfo = $cmddef->{$cmd} if defined $cmd;
if (defined($cmdinfo) && ref($cmdinfo) eq 'HASH') {
if (defined(my $alias = $cmdinfo->{alias})) {
$cmd = $alias;
}
}
> + $args->{base} //= $cmddef;
> + $args->{prefix} //= $exename;
> + if (defined($args->{cmd})) {
> + # If cmd is given, yet base accordingly
Typo (yet)
> + my @cmds = split(' ', $args->{cmd});
> + $args->{prefix} .= " $args->{cmd}";
> + while (@cmds) {
> + $args->{base} = $args->{base}->{shift @cmds};
> + }
> + }
> + $args->{pwcallback} //= $cli_handler_class->can('read_password');
> + $args->{stringfilemap} //=
> $cli_handler_class->can('string_param_file_mapping');
As mentioned above, these two shouldn't be in $args.
> + $args->{sect_sep} //= "";
> + $args->{indent} //= "";
> +
> + my $str = "";
> + if (ref($args->{base}) eq 'HASH') {
Here you start indenting for no apparent reason?
> + my $oldclass = undef;
> + foreach my $cmd ($args->{sortfunc}->($args->{base})) {
> + if (ref($args->{base}->{$cmd}) eq 'ARRAY') {
> + # $cmd is an array, so it's an actual command
> + my ($class, $name, $arg_param, $fixed_param) =
> @{$args->{base}->{$cmd}};
> + $str .= $args->{sect_sep} if $oldclass && $oldclass ne
> $class;
> + $str .= $args->{indent};
> + $str .= $class->usage_str($name, "$args->{prefix} $cmd",
> $arg_param,
> + $fixed_param, $args->{format},
> $args->{pwcallback},
> + $args->{stringfilemap});
> + $oldclass = $class;
> + } elsif (defined($args->{base}->{$cmd}->{alias}) &&
> ($args->{format} eq 'asciidoc')) {
> + # Handle asciidoc separately
> + $str .= "*$args->{prefix} $cmd*\n\nAn alias for '$exename
> $args->{base}->{$cmd}->{alias}'.\n\n";
> + } else {
> + # $cmd has sub-commands or is an alias
> + next if $args->{base}->{$cmd}->{alias};
> + my $substr .= $generate_usage_str->({
Typo (.=)
> + format => $args->{format},
> + sortfunc => $args->{sortfunc},
> + base => $args->{base}->{$cmd},
> + prefix => "$args->{prefix} $cmd",
> + pwcallback => $args->{pwcallback},
> + stringfilemap => $args->{stringfilemap},
> + sect_sep => $args->{sect_sep},
> + indent => $args->{indent},
> + });
Probably nicer to make a shallow copy $args and modify it:
my $rec_args = {%$args};
$rec_args->{prefix} .= " $cmd";
$rec_args->{base} = $args->{base}->{$cmd};
$generate_usage_str->($rec_args);
> + if ($substr) {
> + $substr .= $args->{sect_sep} if $substr !~
> /$args->{sect_sep}$args->{sect_sep}$/;
> + $str .= $substr;
> + }
> + }
> + }
> + } else {
> + # Handle simple commands
> + my ($class, $name, $arg_param, $fixed_param) = @{$args->{base} ||
> []};
> +
> + if (!$class) {
> + print_usage_short (\*STDERR, "unknown command '" . join(' ',
> $args->{cmd}) . "'");
> + exit (-1);
> + }
> +
> + $str .= $args->{indent};
> + $str .= $class->usage_str($name, $args->{prefix}, $arg_param,
> $fixed_param,
> + $args->{format}, $args->{pwcallback},
> $args->{stringfilemap});
> + }
> + return $str;
> +};
> +
> __PACKAGE__->register_method ({
> name => 'help',
> path => 'help',
> @@ -90,18 +168,24 @@ __PACKAGE__->register_method ({
> return undef;
> }
>
> - $cmd = &$expand_command_name($cmddef, $cmd);
> -
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd} || []};
> -
> - raise_param_exc({ cmd => "no such command '$cmd'"}) if !$class;
> + my $base = $cmddef;
> + my @cmds = split(/ /, $cmd);
This should be / +/. But then again, the help command seems to now have
special cased parameter handling and takes a string, so currently doing
`pct help "he asdfxyz"` now shows me the help output for "help" (which
isn't actually a problem, just something to note - the way help walks
through the sub-commands should probably catch this?).
Anyway, we also already have an 'extra-args' special argument type which
could be used instead here to get the parameters as array in
$param->{'extra-args'}.
Just add
'extra-args' => {
type => 'array',
items => { type => 'string' },
optional => 1,
},
to help's parameter definition (can't use get_standard_option() here
yet unfortunately).
Also, $handle_cmd needs to take it into account in its hack:
$cmddef->{help} = [ __PACKAGE__, 'help', ['cmd', 'extra-args'] ];
> + my @newcmd;
> + while (scalar(@cmds) > 0) {
> + # Auto-complete command
> + last if (ref($base) eq 'ARRAY');
> + push @newcmd, &$expand_command_name($base, shift @cmds);
> + $base = $base->{$newcmd[-1]};
> + }
> + $cmd = join(' ', @newcmd);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap =
> $cli_handler_class->can('string_param_file_mapping');
> + my $str = &$generate_usage_str({
> + format => $verbose ? 'full' : 'short',
> + cmd => $cmd,
> + indent => $verbose ? '' : ' ' x 7,
> + });
> + $str =~ s/^\s+//;
>
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param,
> $uri_param,
> - $verbose ? 'full' : 'short', $pwcallback,
> - $stringfilemap);
> if ($verbose) {
> print "$str\n";
> } else {
> @@ -113,17 +197,10 @@ __PACKAGE__->register_method ({
> }});
>
> sub print_simple_asciidoc_synopsis {
> - my ($class, $name, $arg_param, $uri_param) = @_;
> -
> die "not initialized" if !$cli_handler_class;
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> - my $synopsis = "*${name}* `help`\n\n";
> -
> - $synopsis .= $class->usage_str($name, $name, $arg_param, $uri_param,
> - 'asciidoc', $pwcallback, $stringfilemap);
> + my $synopsis = "*${exename}* `help`\n\n";
> + $synopsis .= &$generate_usage_str({format => 'asciidoc'});
>
> return $synopsis;
> }
> @@ -132,24 +209,11 @@ sub print_asciidoc_synopsis {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> my $synopsis = "";
>
> $synopsis .= "*${exename}* `<COMMAND> [ARGS] [OPTIONS]`\n\n";
>
> - my $oldclass;
> - foreach my $cmd (sort keys %$cmddef) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param,
> - $uri_param, 'asciidoc', $pwcallback,
> - $stringfilemap);
> - $synopsis .= "\n" if $oldclass && $oldclass ne $class;
> -
> - $synopsis .= "$str\n\n";
> - $oldclass = $class;
> - }
> + $synopsis .= &$generate_usage_str({format => 'asciidoc'});
>
> $synopsis .= "\n";
>
> @@ -160,21 +224,11 @@ sub print_usage_verbose {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> print "USAGE: $exename <COMMAND> [ARGS] [OPTIONS]\n\n";
>
> - foreach my $cmd (sort keys %$cmddef) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param,
> $uri_param,
> - 'full', $pwcallback, $stringfilemap);
> - print "$str\n\n";
> - }
> -}
> + my $str = &$generate_usage_str({format => 'full'});
>
> -sub sorted_commands {
> - return sort { ($cmddef->{$a}->[0] cmp $cmddef->{$b}->[0]) || ($a cmp
> $b)} keys %$cmddef;
> + print "$str\n";
> }
>
> sub print_usage_short {
> @@ -182,22 +236,54 @@ sub print_usage_short {
>
> die "not initialized" if !($cmddef && $exename && $cli_handler_class);
>
> - my $pwcallback = $cli_handler_class->can('read_password');
> - my $stringfilemap = $cli_handler_class->can('string_param_file_mapping');
> -
> print $fd "ERROR: $msg\n" if $msg;
> print $fd "USAGE: $exename <COMMAND> [ARGS] [OPTIONS]\n";
>
> - my $oldclass;
> - foreach my $cmd (sorted_commands()) {
> - my ($class, $name, $arg_param, $uri_param) = @{$cmddef->{$cmd}};
> - my $str = $class->usage_str($name, "$exename $cmd", $arg_param,
> $uri_param, 'short', $pwcallback, $stringfilemap);
> - print $fd "\n" if $oldclass && $oldclass ne $class;
> - print $fd " $str";
> - $oldclass = $class;
> - }
> + print &$generate_usage_str({format => 'short', sect_sep => "\n",
> sortfunc =>
This misses the $fd.
> + sub {
> + my ($hash) = @_;
> + return sort {
> + if ((ref($hash->{$a}) eq 'ARRAY' && ref($hash->{$b}) eq
> 'ARRAY') &&
> + ($hash->{$a}->[0] ne $hash->{$b}->[0])) {
> + # If $a and $b are both arrays (commands) and the commands
> are not in
> + # the same class, order their classes alphabetically
> + return $hash->{$a}->[0] cmp $hash->{$b}->[0];
> + } elsif (ref($hash->{$a}) eq 'ARRAY' xor ref($hash->{$b}) eq
> 'ARRAY') {
> + # If one is an array (command) and one is a hash (has
> subcommands),
> + # sort commands behind sub.commands
> + return ref($hash->{$b}) eq 'ARRAY' ? -1 : 1;
> + } else {
> + # If $a and $b are both commands of the same class or both
> sub-commands,
> + # sort alphabetically
> + return $a cmp $b;
> + }
> + } keys %$hash;
> + }, indent => ' ' x 7});
> }
>
> +my $print_help_short = sub {
> + my ($fd, $cmd, $msg) = @_;
> +
> + die "not initialized" if !($cmddef);
> +
> + print $fd "ERROR: $msg\n" if $msg;
> +
> + my $base = $cmddef;
> + while (scalar(@$cmd) > 1) {
> + $base = $base->{shift @$cmd};
> + }
> +
> + my $str = &$generate_usage_str({
> + format => 'short',
> + base => $base,
> + cmd => $cmd->[0],
> + indent => ' ' x 7,
> + });
> + $str =~ s/^\s+//;
> +
> + print "USAGE: $str\n";
> +};
> +
> my $print_bash_completion = sub {
> my ($cmddef, $simple_cmd, $bash_command, $cur, $prev) = @_;
>
> @@ -225,17 +311,40 @@ my $print_bash_completion = sub {
> };
>
> my $cmd;
> + my $def = $cmddef;
> + my $cmd_depth = 0;
> + if (scalar(@$args) > 1) {
> + for my $i (1 .. $#$args) {
> + last if (ref($def) eq 'ARRAY');
> + if (@$args[$i] ne $cur && exists $def->{@$args[$i]}) {
> + # Move def to proper sub-command-def
> + # Don't try yet-to-complete commands
> + # exists… prevents auto-vivification
> + $def = $def->{@$args[$i]};
> + $cmd_depth++;
> + }
> + }
> + }
> if ($simple_cmd) {
> $cmd = $simple_cmd;
> + $def = $def->{$simple_cmd};
> } else {
> - if ($pos == 0) {
> - &$print_result(keys %$cmddef);
> - return;
> + if (ref($def) eq 'HASH') {
> + if (exists $def->{alias}) {
> + # Move def to aliased command
> + my $newdef = $cmddef;
> + foreach my $subcmd (split(/ /, $def->{alias})) {
> + $newdef = $newdef->{$subcmd};
> + }
> + $def = $newdef;
> + } else {
> + &$print_result(keys %$def);
> + return;
> + }
> }
> - $cmd = $args->[1];
> + $cmd = @$args[-1];
> }
>
> - my $def = $cmddef->{$cmd};
> return if !$def;
>
> print STDERR "CMDLINE1:$pos:$cmdline\n" if $debug;
> @@ -251,12 +360,11 @@ my $print_bash_completion = sub {
> map { $skip_param->{$_} = 1; } @$arg_param;
> map { $skip_param->{$_} = 1; } keys %$uri_param;
>
> - my $fpcount = scalar(@$arg_param);
> + my $fpcount = scalar(@$arg_param) + $cmd_depth - 1;
>
> my $info = $class->map_method_by_name($name);
>
> - my $schema = $info->{parameters};
> - my $prop = $schema->{properties};
> + my $prop = $info->{parameters}->{properties};
>
> my $print_parameter_completion = sub {
> my ($pname) = @_;
> @@ -277,7 +385,7 @@ my $print_bash_completion = sub {
> # positional arguments
> $pos += 1 if $simple_cmd;
> if ($fpcount && $pos <= $fpcount) {
> - my $pname = $arg_param->[$pos -1];
> + my $pname = $arg_param->[$pos - $cmd_depth];
> &$print_parameter_completion($pname);
> return;
> }
> @@ -375,12 +483,11 @@ sub generate_asciidoc_synopsis {
>
> no strict 'refs';
> my $def = ${"${class}::cmddef"};
> + $cmddef = $def;
>
> if (ref($def) eq 'ARRAY') {
> print_simple_asciidoc_synopsis(@$def);
> } else {
> - $cmddef = $def;
> -
> $cmddef->{help} = [ __PACKAGE__, 'help', ['cmd'] ];
>
> print_asciidoc_synopsis();
> @@ -405,33 +512,62 @@ my $handle_cmd = sub {
> # call verifyapi before setup_environment(), because we do not want to
> # execute any real code in this case
>
> - if (!$cmd) {
> + if (!defined($cmd->[0])) {
> print_usage_short (\*STDERR, "no command specified");
> exit (-1);
> - } elsif ($cmd eq 'verifyapi') {
> + } elsif ($cmd->[0] eq 'verifyapi') {
> PVE::RESTHandler::validate_method_schemas();
> return;
> }
>
> $cli_handler_class->setup_environment();
>
> - if ($cmd eq 'bashcomplete') {
> + if ($cmd->[0] eq 'bashcomplete') {
> &$print_bash_completion($cmddef, 0, @$args);
> return;
> }
>
> &$preparefunc() if $preparefunc;
>
> - $cmd = &$expand_command_name($cmddef, $cmd);
> + unshift @$args, shift @$cmd;
> + my $base = $def;
> + while (scalar(@$args) > 0) {
> + last if (ref($base) eq 'ARRAY');
> + # Auto-complete commands
> + push @$cmd, &$expand_command_name($base, shift @$args);
> + $base = $base->{$cmd->[-1]};
> + if (ref($base) eq 'HASH' && defined($base->{alias})) {
> + # If command is an alias, reset $base and move to aliased command
> + my @alias = split(/ /, $base->{alias});
> + $base = $def;
> + undef(@$cmd);
> + while (@alias) {
> + unshift @$args, pop @alias;
> + }
unshift @$args, @alias;
> + }
> + }
> +
> + if (ref($base) eq 'HASH') {
> + &$print_help_short (\*STDERR, $cmd, "incomplete command '" . join(' ',
> @$cmd) . "'");
> + exit (-1);
> + }
>
> - my ($class, $name, $arg_param, $uri_param, $outsub) = @{$cmddef->{$cmd}
> || []};
> + my ($class, $name, $arg_param, $uri_param, $outsub) = @{$base || []};
>
> if (!$class) {
> - print_usage_short (\*STDERR, "unknown command '$cmd'");
> + print_usage_short (\*STDERR, "unknown command '" . join(' ', @$cmd) .
> "'");
> exit (-1);
> + } elsif ($name eq 'help') {
> + # Find command help is wanted for
> + my @help_cmd;
> + while (@ARGV) {
> + last if ($ARGV[0] =~ /^-/);
> + push @help_cmd, shift @ARGV;
> + }
> + unshift @ARGV, join(' ', @help_cmd);
> }
>
> - my $prefix = "$exename $cmd";
> + my $prefix = "$exename " . join(' ', @$cmd);
> my $res = $class->cli_handler($prefix, $name, \@ARGV, $arg_param,
> $uri_param, $pwcallback, $stringfilemap);
>
> &$outsub($res) if $outsub;
> @@ -446,7 +582,7 @@ my $handle_simple_cmd = sub {
> if (scalar(@$args) >= 1) {
> if ($args->[0] eq 'help') {
> my $str = "USAGE: $name help\n";
> - $str .= $class->usage_str($name, $name, $arg_param, $uri_param,
> 'long', $pwcallback, $stringfilemap);
> + $str .= &$generate_usage_str({format => 'long'});
> print STDERR "$str\n\n";
> return;
> } elsif ($args->[0] eq 'verifyapi') {
> @@ -508,13 +644,14 @@ sub run_cli_handler {
>
> no strict 'refs';
> my $def = ${"${class}::cmddef"};
> + $cmddef = $def;
>
> if (ref($def) eq 'ARRAY') {
> &$handle_simple_cmd($def, \@ARGV, $pwcallback, $preparefunc,
> $stringfilemap);
> } else {
> $cmddef = $def;
> - my $cmd = shift @ARGV;
> - &$handle_cmd($cmddef, $exename, $cmd, \@ARGV, $pwcallback,
> $preparefunc, $stringfilemap);
> + my @cmd = shift @ARGV;
> + &$handle_cmd($cmddef, $exename, \@cmd, \@ARGV, $pwcallback,
> $preparefunc, $stringfilemap);
> }
>
> exit 0;
> --
> 2.11.0
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
