CLI: fix shell completion of script for verdi run
#5430
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sphuber
force-pushed
the
fix/verdi-run-shell-complete
branch
from
76a7fef to
8849518
Compare
|
@unkcpz I took the start of your fix and fleshed it out a bit to fix the implementation. I then also added a fix on top to reinstate shell completion for relative filepaths. So I decided to make a separate PR, hope you don't mind. |
|
@sphuber absolutely not mind 😄. I'll have a review and test the change later.
|
sphuber
force-pushed
the
fix/verdi-run-shell-complete
branch
from
8849518 to
c9e2ee2
Compare
unkcpz
requested changes
Mar 14, 2022
aiida/cmdline/commands/cmd_verdi.py
Outdated
| return False | ||
|
|
||
| # Allow characters that typically designate the start of a path. | ||
| return not value[0].isalnum() and value[0] not in ['/', '.', '~'] |
There was a problem hiding this comment.
Suggested change
| return not value[0].isalnum() and value[0] not in ['/', '.', '~'] | |
| return not value[0].isalnum() and value[0] not in ['/', '.', '~', '$'] |
I think should also add to escape for $HOME and $PWD here.
When we moved to the built-in shell completion of `click==8.0` instead of the external `click_completion`, the script argument of `verdi run` was no longer auto-completed. The reason is that `click` does not autocomplete files for the `STRING` type. The parameter type is changed to `click.Path`, which not only fixes the problem of auto-completion, it also provides automatic validation by making sure if it is an existing file and not a directory. This allows to simplify the logic in the command body significantly as well as we no longer need to handle the `IOError` of unreadable files manually.
Since `click==8.0` started shipping with their own implementation of
shell completion, we started using that instead of `click_completion`.
However, `click`'s implementation currently does not auto-complete
relative filepaths that start, for example, with either `.`, `~` or `$`.
They made an exception for the `/` character, in the following PR:
pallets/click#1930
Other users have noticed the problem with relative filepaths:
pallets/click#2040
But it is not sure if they plan to support this anytime soon so we
simply monkeypatch `click` ourselves by changing the implementation for
the `click.shell_completion._start_of_option`.
sphuber
force-pushed
the
fix/verdi-run-shell-complete
branch
from
c9e2ee2 to
1ad1d76
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we moved to the built-in shell completion of
click==8.0insteadof the external
click_completion, the script argument ofverdi runwas no longer auto-completed. The reason is that
clickdoes notautocomplete files for the
STRINGtype.The parameter type is changed to
click.Path, which not only fixes theproblem of auto-completion, it also provides automatic validation by
making sure if it is an existing file and not a directory. This allows
to simplify the logic in the command body significantly as well as we no
longer need to handle the
IOErrorof unreadable files manually.