CLI: Raise if no root node specified in verdi node graph generate
#6405
Conversation
The `root_node` positional argument was recently made optional since it was deprecated and replaced by the `-N/--nodes` option. However, the change forgot to check that at least one root node is specified. Not specifying any would result in an exception.
sphuber
added
the
priority/critical-blocking
|
Labeling as blocking since this bug was introduced by changes that have not yet been released |
| @@ -510,6 +510,9 @@ def graph_generate( | |||
| ) | |||
| root_node = None | |||
|
|
|||
| if not root_node and not nodes: | |||
| echo.echo_critical('No root node specified, please specify one or more using the `-N/--nodes` option.') | |||
|
|
|||
| if root_node: | |||
| echo.echo_deprecated( | |||
There was a problem hiding this comment.
Unrelated to the fix, but is it really necessary to deprecate the original behaviour? Supposedly this is the most common use case, so why not allow this shorthand.
(Apologies if this was already discussed ignore me in that case)
There was a problem hiding this comment.
Because it would be potentially confusing if there are two methods. If we were to keep it, the user wanting to specify multiple nodes would still have to specify the root node positional argument, e.g., verdi node graph generate -N 10 11 -- 15 output.pdf. I guess we could either change the order of ROOT_NODE and OUTPUT_FILE or change the output file from an argument to an option. But in both cases we would be changing the interface.
One other solution perhaps (if we are thinking of changing the interface anyway) would be to get rid of the -N option, turn output file from argument to an option, and allow root nodes to be specified as positional arguments. So example uses would be
verdi node graph generate 10
verdi node graph generate 10 11
verdi node graph generate -o output.pdf 10
verdi node graph generate -o output.pdf 10 11
Maybe that would be cleanest?
Most of the verdi node commands that take more than one node use arguments. But there are also examples where the -N/--nodes option is used, but those are limited. I think in most cases using arguments would make most sense. The reason verdi archive create uses an option is because you can also specify groups and other entities, so there it makes sense. Maybe using arguments in verdi node graph generate really makes most sense.
This is what click has to say in their docs about options vs arguments:
Click supports two types of parameters for scripts: options and arguments. There is generally some confusion among authors of command line scripts of when to use which, so here is a quick overview of the differences. As its name indicates, an option is optional. While arguments can be optional within reason, they are much more restricted in how optional they can be.
To help you decide between options and arguments, the recommendation is to use arguments exclusively for things like going to subcommands or input filenames / URLs, and have everything else be an option instead.
The first paragraph would suggest we use an argument since at least one root node is required and not optional. However, the second paragraph would suggest using an option, but maybe our use case is a bit more specific compared to most CLI tools. The output filename would seem to fit in the argument category, but could also be argued to be added as an option, since the command will generate an output file by default.
TLDR: Maybe the best solution would be to move output file to an option and specify the root nodes with arguments?
There was a problem hiding this comment.
@danielhollas I opened a new category on discourse for design discussions. Opened a first topic on the question in this PR. Seems that people are gravitating to using arguments to allow specifying root nodes and use an option for the optional output file. Let us know what you think please: https://aiida.discourse.group/t/redesign-of-the-verdi-node-graph-generate-command/395
There was a problem hiding this comment.
Replied on Discourse, thanks!
|
Thanks for the review @agoscinski but given the responses on the discourse thread it seems like we would rather be going with a different solution, as implemented in #6427 |
|
Superseded by #6427 |
Fixes #6397
The
root_nodepositional argument was recently made optional since it was deprecated and replaced by the-N/--nodesoption. However, the change forgot to check that at least one root node is specified. Not specifying any would result in an exception.