-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show the default for switch options in transport CLI #4223
Show the default for switch options in transport CLI #4223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @sphuber !
I can't say I fully understand what's going on with the different defaults here but I've checked that it fixes the help strings (and that interactive configuration still works as well).
aiida/transports/cli.py
Outdated
) | ||
non_interactive_default = spec.pop('non_interactive_default', False) | ||
kwargs['default'] = non_interactive_default | ||
kwargs['show_default'] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this then not just move outside the if/else?
a098aae
to
b10a5be
Compare
The options for the `verdi computer configure` command are created dynamically based on the `_valid_connect_options` and the `_valid_auth_options` class attributes of the transport plugin class. These are interactive options whose defaults are context based, meaning they can be defined by a previously existing transport configuration. However, the "default" defaults, if you will, i.e. the defaults when the computer has never been configured before, were not defined, so they were also not printed in the help message string of the command. We know explicitly define these base defaults.
b10a5be
to
66bc488
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4223 +/- ##
===========================================
+ Coverage 79.17% 79.17% +0.01%
===========================================
Files 468 468
Lines 34467 34464 -3
===========================================
- Hits 27285 27284 -1
+ Misses 7182 7180 -2
Continue to review full report at Codecov.
|
I agree that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sphuber , the code has certainly gotten a lot more readable.
I checked that the defaults are still shown and that the configuration still works.
I wonder whether we would like to show the default values also for a couple of other options that now seem treated differently
$ verdi computer configure ssh -h
Usage: verdi computer configure ssh [OPTIONS] COMPUTER
Configure COMPUTER for ssh transport.
Options:
...
-P, --port INTEGER Port number.
...
--key-filename FILE Absolute path to your private SSH key. Leave
empty to use the path set in the SSH config.
--timeout INTEGER Time in seconds to wait for connection
before giving up. Leave empty to use default
value.
...
...
--safe-interval FLOAT Minimum time interval in seconds between
opening new connections.
The autogenerated command line interfaces for transport plugins were not
setting
show_default
toTrue
, even if anon_interactive_default
isspecified, which means that the help string would not say which of the
two switch values is the default.