Re: [libvirt] [jenkins-ci PATCH v4 1/5] lcitool: use subparsers for commands
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