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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
