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

Feat/load completion improvements #924

Merged
merged 16 commits into from
Apr 22, 2023
Merged

Conversation

scop
Copy link
Owner

@scop scop commented Apr 10, 2023

Intended to be merged after #923.

This started from looking into #917 (comment) and ballooned in scope somewhat. The gist of this is that we now provide the full command name (even when not invoked with one) to the completions when sourced, to facilitate generating from the correct executable, and for improved handling of commands invoked with relative paths.

The commits are kind of intertwined, but I hope looking into them individually for details provides enough context if the end result is hard to follow.

scop added 7 commits April 10, 2023 16:41
Previously, we handled it at the start of the command's _basename_. Even
though the backslash doesn't make a practical difference with regards to
aliases with paths, it seems more logical and clearer to handle it this
way.

When invoked as just the basename, this does not change anything.
Passing just the basename results in failures when a command invoked
(with a path) is not in `$PATH`.

Also facilitates correct command instance being used to generate the
completion in our fallback completion loaders. Completions generated by
different instances of the command might vary.
For consistency and completeness, e.g. arg #'s > 1 for fallback loaders,
and the same (possibly processed) command.
Mostly for better handling of completions loaded via relative paths, in
case the completion registers it for the exact command passed to it.
On successful return, at least the full path to the command in question
should have a completion set.

If the command invoked was given without a path, the pathless one should
have one set, too.
@scop scop force-pushed the feat/load-completion-improvements branch from 23ee38b to f16dd80 Compare April 10, 2023 19:22
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this issue! This is great.

bash_completion Outdated Show resolved Hide resolved
bash_completion Outdated Show resolved Hide resolved
@akinomyoga
Copy link
Collaborator

I think we should also fix

diff --git a/completions/_nox b/completions/_nox
index ac387ef7..8e98002a 100644
--- a/completions/_nox
+++ b/completions/_nox
@@ -9,6 +9,8 @@ eval -- "$(
         register-python-argcomplete3 --shell bash "$1" 2>/dev/null
 )"

-complete -p "$1" &>/dev/null
+{
+    complete -p "$1" || complete -p "${1##*/}"
+} &>/dev/null

 # ex: filetype=sh

@scop
Copy link
Owner Author

scop commented Apr 11, 2023

-complete -p "$1" &>/dev/null
+{
+    complete -p "$1" || complete -p "${1##*/}"
+} &>/dev/null

I'm not quite sure about it: I'm not aware of any version of register-python-argcomplete that would register the completion for anything else besides the exact command passed to it. Then again I think we would not even be loading this completion for a full path one if the basename already had a completion installed (because it would have been just run, instead of being looked up), so in that sense the suggested change would not be a problem.

But actually, I thought about it but forgot: this stuff should no longer be necessary here in the first place, because __load_completion now checks if a completion actually got installed anyway. 95eef92 and a44ba64 gets rid of the no longer needed bits.

Base automatically changed from refactor/comp-realcommand to master April 11, 2023 19:43
scop and others added 3 commits April 11, 2023 22:52
This might make the path we install completions for different from the
invoked path so that the completion would fail to actually load after
setup.

The final path segment might also have a different name in the resolved
symlink, and we should not install it for the resolved name, because the
target's functionality might vary based on the invocation name.

Simply replacing the final path segment with the original basename could
yield a path that does not exist, even though the original invoked one
does.

#924 (comment)
bash_completion Outdated Show resolved Hide resolved
bash_completion Show resolved Hide resolved
bash_completion Show resolved Hide resolved
bash_completion Outdated Show resolved Hide resolved
@akinomyoga akinomyoga force-pushed the feat/load-completion-improvements branch from 81132e7 to 01e1ae5 Compare April 17, 2023 20:49
@scop scop merged commit 787ad5c into master Apr 22, 2023
@scop scop deleted the feat/load-completion-improvements branch April 22, 2023 12:55
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