[libvirt] [jenkins-ci PATCH v4 1/5] lcitool: use subparsers for commands

[Andrea Bolognani]

On Thu, 2019-02-21 at 16:33 +0000, Daniel P. Berrangé wrote:
> Currently only a single global parser is used for all commands. This
> means that every command accepts every argument which is undesirable as
> users don't know what to pass. It also prevents the parser from
> generating useful help information.
> 
> Python's argparse module supports multi-command binaries in a very easy
> way by adding subparsers, each of which has their own distinct
> arguments. It can then generate suitable help text on a per command
> basis. This also means commands can use positional arguments for data
> items that are always passed.
> 
>     $ ./guests/lcitool -h
>     usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...
> 
>     libvirt CI guest management tool
> 
>     positional arguments:
>       {install,update,build,hosts,projects,dockerfile}
>                             commands
>         install             perform unattended host installation
>         update              prepare hosts and keep them updated
>         build               build projects on hosts
>         hosts               list all known hosts
>         projects            list all known projects
>         dockerfile          generate a host docker file
> 
>     optional arguments:
>       -h, --help            show this help message and exit
> 
>     $ ./guests/lcitool install -h
>     usage: lcitool install [-h] hosts
> 
>     positional arguments:
>       hosts       list of hosts to act on (accepts globs)
> 
>     optional arguments:
>       -h, --help  show this help message and exit
> 
>     $ ./guests/lcitool dockerfile  -h
>     usage: lcitool dockerfile [-h] hosts projects
> 
>     positional arguments:
>       hosts       list of hosts to act on (accepts globs)
>       projects    list of projects to consider (accepts globs)
> 
>     optional arguments:
>       -h, --help  show this help message and exit

I feel like the usage examples are not really adding much to the
commit message, so I'd just leave them out.

On the other hand, all the examples in README.markdown are now
invalid, so you should make sure that file is updated along with
the script.

[...]
> +++ b/guests/lcitool
> @@ -307,43 +307,72 @@ class Application:
>              conflict_handler="resolve",
>              formatter_class=argparse.RawDescriptionHelpFormatter,

Since we're no longer providing our own, pre-formatted help text
and we're leaving its generation up to argparse instead, setting
formatter_class is no longer needed.

The same applies to all callers you introduce later.

[...]
> +        subparser = self._parser.add_subparsers(help="commands")

Please rename the variable to 'subparsers'. At the same time, drop
use of the 'help' parameter and set 'metavar' to 'ACTION' instead:
that will change the main help output from

  usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...

  libvirt CI guest management tool

  positional arguments:
    {install,update,build,hosts,projects,dockerfile}
                          commands
      install             perform unattended host installation
      ...

to a less unwieldy

  usage: lcitool [-h] ACTION ...

  libvirt CI guest management tool

  positional arguments:
    ACTION
      install   perform unattended host installation
      ...

As an aside, I don't much like how argparse takes over the '-h'
option as a shorthand for '--help', especially since up until now
it has been used to specify hosts, but I haven't found a way to
disable that behavior. I guess I can live with it :)

> +        subparser.required = True
> +        subparser.dest = "command"

Setting the 'dest' parameter is not necessary - you're not using
it anywhere.

[...]
> +        installparser.set_defaults(func=self._action_install)

Well, isn't this clever :)

[...]
> +        dockerfileparser = subparser.add_parser(
> +            "dockerfile", help="generate a host docker file",

I'd prefer if you kept the original help text here.


Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization