Use correct executable name for completion command #4755
Conversation
|
Oops, I missed that there was an open PR for this. 🙁 For what it's worth, I think this approach is better, because it a) reuses an existing utility function b) fixes a bug in said utility function and c) adds tests. |
I agree. PS: I modified the OP to also close the other PR. |
| @@ -80,7 +83,9 @@ def run(self, options, args): | |||
| shell_options = ['--' + shell for shell in sorted(shells)] | |||
| if options.shell in shells: | |||
| script = textwrap.dedent( | |||
| COMPLETION_SCRIPTS.get(options.shell, '') | |||
| COMPLETION_SCRIPTS.get(options.shell, '').format( | |||
There was a problem hiding this comment.
Why not just simply do a % formatting; for consistency?
There was a problem hiding this comment.
Great suggestion, thanks.
| """, | ||
| 'fish': """ | ||
| function __fish_complete_pip | ||
| set -lx COMP_WORDS (commandline -o) "" | ||
| set -lx COMP_CWORD {cword} | ||
| set -lx COMP_CWORD ( \\ |
There was a problem hiding this comment.
Pointing out that this line changed and I've not verified that this change works; I'm pretty sure it would but yeah.
There was a problem hiding this comment.
I have verified that this still works (I also learned that fish shell is pretty nice!). I changed this line because the .format was unnecessary, but these lines still need to be < 80 characters.
There was a problem hiding this comment.
Yeah. I'd added the .format as a hack for making the lines < 80 characters in #4509. I intended to come around to it whenever I would have found the time to fiddle around with fish -- a duration long enough to do it justice anyway. :P
I also learned that fish shell is pretty nice!
It is indeed.
|
FYI, we've just merged in pip completions using pip's completion mechanism on the fish project, (fish-shell/fish-shell#4448) and ran into what this PR will fix. Happy to see it's in the works! |
|
(Any idea why it's so slow to output anything? ... it causes some lag when tab completing.) |
|
Hi @floam! I'd speculate that is mostly startup time for pip and Python combined? The latter is something a lot of people are working on right now and the former is indeed something that I'm planning to look into eventually. |
|
Hm, maybe 35ms is Python starting up itself, the other 600ms appears to occur in pip. Anyhow, off topic for this PR, sorry! |
Work around bug pypa/pip#4755 Don't expect all users to be running a version of pip2/3 that includes the fix (once it's upstreamed). Will continue to work if/when pip2/3 emit the correct output. pip is already very slow at printing the completions (see #4448) so the `sed` call overhead is neglible.
Work around bug pypa/pip#4755 Don't expect all users to be running a version of pip2/3 that includes the fix (once it's upstreamed). Will continue to work if/when pip2/3 emit the correct output. pip is already very slow at printing the completions (see #4448) so the `sed` call overhead is neglible.
|
@floam I also noticed this was slow when testing it -- I'd suggest you make a separate issue so it can be addressed. |
|
@di I think there is one for it... |
|
There was #3360 |
|
Thanks @xavfernandez! ^.^ I guess, in that case, I'll make a new issue then. (edit: #4768) |
|
This LGTM. Could another @pypa/pip-committers take a look? |
|
Thanks @xavfernandez! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
lock
bot
added
the
auto-locked
Fixes #3997.
Closes #4014.