Skip to content
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

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jul 6, 2020

The autogenerated command line interfaces for transport plugins were not
setting show_default to True, even if a non_interactive_default is
specified, which means that the help string would not say which of the
two switch values is the default.

ltalirz
ltalirz previously approved these changes Jul 6, 2020
Copy link
Member

@ltalirz ltalirz left a 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).

)
non_interactive_default = spec.pop('non_interactive_default', False)
kwargs['default'] = non_interactive_default
kwargs['show_default'] = True
Copy link
Member

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?

@sphuber sphuber force-pushed the fix/4221/cli-transport-configure-switch-defaults branch from a098aae to b10a5be Compare July 6, 2020 15:15
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.
@sphuber sphuber force-pushed the fix/4221/cli-transport-configure-switch-defaults branch from b10a5be to 66bc488 Compare July 6, 2020 17:03
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #4223 into develop will increase coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             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     
Flag Coverage Δ
#django 71.09% <87.50%> (+0.01%) ⬆️
#sqlalchemy 71.93% <87.50%> (+0.01%) ⬆️
Impacted Files Coverage Δ
aiida/transports/plugins/ssh.py 76.39% <ø> (ø)
aiida/transports/cli.py 93.34% <87.50%> (+0.87%) ⬆️
aiida/transports/plugins/local.py 80.47% <0.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e5d5e0...66bc488. Read the comment docs.

@sphuber sphuber requested a review from ltalirz July 6, 2020 17:43
@sphuber
Copy link
Contributor Author

sphuber commented Jul 6, 2020

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).

I agree that the non_interactive_default key was not very clear, it also took me quite a while to figure out the original intention. I have updated the code and docstrings a bit to hopefully clarify its functioning. I have dismissed your first review since the code changed quite a bit. Would be good if you could test again that help strings are correct and that interactive works.

Copy link
Member

@ltalirz ltalirz left a 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.

@sphuber sphuber merged commit 1a4cada into aiidateam:develop Jul 7, 2020
@sphuber sphuber deleted the fix/4221/cli-transport-configure-switch-defaults branch July 7, 2020 09:45
@sphuber sphuber added this to the v1.3.1 milestone Jul 10, 2020
@sphuber sphuber modified the milestones: v1.3.1, v1.3.2 Aug 27, 2020
@sphuber sphuber modified the milestones: v1.3.2, v1.4.0 Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants