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

Python's pip and pipenv completions #4448

Merged
merged 5 commits into from
Oct 5, 2017
Merged

Conversation

cprieto
Copy link
Contributor

@cprieto cprieto commented Oct 4, 2017

pip and pipenv completions

Both pip and pipenv has its own completion support for fish, so we only call their native completion support if the command is installed in the path, nothing fancy but this avoids having to manually install the completion.

 * We call native pip completion for fish if pip is installed
 * We call pipenv native completion if pipenv is installed
@@ -0,0 +1,3 @@
if test (command -v pipenv)
eval (pipenv --completion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any guarantees that --completion will always be fish-compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is guarantee to always return completion for the shell pointed by the SHELL environment variable. This is the way is documented in the official documentation and I checked the source code (of Pipenv) as well.

Copy link
Member

@floam floam Oct 5, 2017

Choose a reason for hiding this comment

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

It's certainly producing fish-compatible output intended to be eval'd.

 $ pipenv --completion
complete --command pipenv --arguments "(env _PIPENV_COMPLETE=complete-fish COMMANDLINE=(commandline -cp) pipenv)" -f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I checked the source code for pipenv and click-completion before submitting the patch. Pretty good code if you ask me.

The only concern will be if you set SHELL as something that is not fish, but I think in that case not only this completion will fail.

@@ -0,0 +1,3 @@
if test (command -v pip)
Copy link
Member

Choose a reason for hiding this comment

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

This can just be if command -sq pip

@@ -0,0 +1,3 @@
if test (command -v pipenv)
Copy link
Member

Choose a reason for hiding this comment

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

This can just be if command -sq pipenv

 * Changed usage of `test (command -v)` for just `command -sq`
@floam floam added this to the fish 2.7.0 milestone Oct 5, 2017
@floam floam self-assigned this Oct 5, 2017
@floam
Copy link
Member

floam commented Oct 5, 2017

On my system, I notice there is only pip2 and pip2.7 - no pip. How do we want to handle this? @faho? A symlink? Git handles them fine.

@floam
Copy link
Member

floam commented Oct 5, 2017

That won't just work, of course, the function name would be wrong. I guess the one file could have multiple functions in it.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 5, 2017

@floam I actually thought about it (in my system I have both pip2 and pip3 and depending of the virtualenv used it changes and create an alias for the correct pip pointed to your Python version). In fact, the first version I did had the functions pip2 and pip3 for that case but I thought it will be too much. If the scenario where both pip exists and are not aliased by the virtualenv it will make sense, though I have no idea how common that scenario would be (in my Mac and Ubuntu both look to do the same pip2/3 aliasing to pip).

should I add the same conditions for both?

 * In some systems pip is not aliased and we have pip2 and pip3
 * In those cases, we just load the completions for those commands
@floam
Copy link
Member

floam commented Oct 5, 2017

You'll need to add a pip2.fish and a pip3.fish - either symlinked or just copies with a function each.

@floam floam merged commit 1770e47 into fish-shell:master Oct 5, 2017
floam pushed a commit that referenced this pull request Oct 5, 2017
* Add pip completion

 * We call native pip completion for fish if pip is installed

* Add pipenv completion

 * We call pipenv native completion if pipenv is installed

* Applied changes as requested by @floam

 * Changed usage of `test (command -v)` for just `command -sq`

* Add completions for pip2/3

 * In some systems pip is not aliased and we have pip2 and pip3
 * In those cases, we just load the completions for those commands

* Separate pip2/3 completions in their own file as requested by @floam
@floam
Copy link
Member

floam commented Oct 5, 2017

Merged, thanks!

@floam
Copy link
Member

floam commented Oct 5, 2017

Hm, I do notice something keeping pip2 completions from working here, but this ought to be addressed upstream:

$ pip2 completion --fish

# pip fish completion start
function __fish_complete_pip
    set -lx COMP_WORDS (commandline -o) ""
    set -lx COMP_CWORD (math (contains -i -- (commandline -t) $COMP_WORDS)-1)
    set -lx PIP_AUTO_COMPLETE 1
    string split \  -- (eval $COMP_WORDS[1])
end
complete -fa "(__fish_complete_pip)" -c pip
# pip fish completion end

Running pip2 completion --fish outputs a complete command for pip, not pip2.

@floam
Copy link
Member

floam commented Oct 5, 2017

Any chance you could report this there?

@cprieto
Copy link
Contributor Author

cprieto commented Oct 5, 2017

Sure, I will open the ticket at pip, I was afraid something like this could happen. Let me see what can be done.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 5, 2017

I think I have a simple quick way to solve this and submit a patch to pip, let me work on that this arvo taking advantage I don't have much to do.

@floam
Copy link
Member

floam commented Oct 5, 2017

It's also rather unfortunate that pip is quite slow to output the code, causing a noticeable lag on hitting tab:

$ pip2 completion --fish > /dev/null
$ echo $CMD_DURATION
636

... but there's not a lot we here can do about that.

@floam
Copy link
Member

floam commented Oct 5, 2017

Hey, there's already an open PR over there for the command name thing:

pypa/pip#4755

@cprieto
Copy link
Contributor Author

cprieto commented Oct 5, 2017

Yeah I saw the patch before leaving (right now on a tram in my way home). Should we wait and see?

@floam
Copy link
Member

floam commented Oct 5, 2017

Yep.

mqudsi added a commit that referenced this pull request Oct 5, 2017
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.
mqudsi added a commit that referenced this pull request Oct 5, 2017
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.
@mqudsi
Copy link
Contributor

mqudsi commented Oct 5, 2017

Fixed in b11ca2c and 3f9273c

@floam
Copy link
Member

floam commented Oct 5, 2017

How will that continue to work when they fix it, @mqudsi? Won't it cause -c pip2 -> -c pip22?

mqudsi added a commit that referenced this pull request Oct 5, 2017
Thanks @floam. See #4448

(cherry picked from commit 78fe534)
@mqudsi
Copy link
Contributor

mqudsi commented Oct 5, 2017

Thanks. Corrected and pushed.

@floam
Copy link
Member

floam commented Oct 5, 2017

Ok, it still doesn't work. I assumed that was it. Did you test it? I don't think the regex is correct for PCRE. Might want to check the documentation they have. Sorry for saying it looked good.

I'm getting complete -fa "(__fish_complete_pip)" -c pip.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 5, 2017

Ugh, I tested it.. only my test case emitted a plain pip in the first place :/ Sorry.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 5, 2017

It's the string replace that's the issue. I have to put both the pattern and the completion after --. I had originally tested it with sed -r and then I botched the test with string replace.

@floam
Copy link
Member

floam commented Oct 5, 2017

pip2 completion --fish | string replace -r -- "\b-c\s+pip\b" "-c pip2" also prints output with -c pip here.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 5, 2017

Word boundaries don't match words starting with -, which is... annoying.

mqudsi@ZBook ~/r/fish-shell> echo "hello -c pip something" | string replace -r -- ' -c\s+pip\b' ' -c pip3'
hello -c pip3 something

Fixed and pushed (yet again).

@floam
Copy link
Member

floam commented Oct 5, 2017

Looks like --disable-pip-version-check speeds it up spitting out code a little bit, around 100ms. Still unpleasantly slow, though.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 5, 2017

I will take a look at that with the python profiler tomorrow. Thanks @floam

@faho
Copy link
Member

faho commented Oct 6, 2017

Soo... if the output of pip completion --fish looks like this:

# pip fish completion start
function __fish_complete_pip
    set -lx COMP_WORDS (commandline -o) ""
    set -lx COMP_CWORD (math (contains -i -- (commandline -t) $COMP_WORDS)-1)
    set -lx PIP_AUTO_COMPLETE 1
    string split \  -- (eval $COMP_WORDS[1])
end
complete -fa "(__fish_complete_pip)" -c pip

Then why are we bothering with the source thing at all? Why not just include that function directly? (Also, I'd replace the eval $COMP_WORDS[1] with a direct call to pip or pip2 or pip3)

@floam
Copy link
Member

floam commented Oct 6, 2017

I'm not sure there's any guarantee the interface will remain stable. There are open PRs there that add new features and change the function IIRC. Since they're providing the feature officially through pip completion it seems wise to me to just let them handle it.

@faho
Copy link
Member

faho commented Oct 6, 2017

That current interface is a direct translation to the bash completion system, so it kinda has to remain stable. In case they add any fish-specific features, we can remove this again, or conditionalize it on the version.

@floam
Copy link
Member

floam commented Oct 6, 2017

Hm, pip takes like 500ms just to tell you what version it is. And it seems way more future-proof just to let it do its thing.

@faho
Copy link
Member

faho commented Oct 6, 2017

I'm sorry, I don't follow? By using the function directly, we can remove that first call to pip, so we're 500ms faster.

Or did I not have enough coffee this morning?

@floam
Copy link
Member

floam commented Oct 6, 2017

Right, I just mean that a version check would render useless the optimization, and mean we have to do the work to adjust down the road if anything changed.

@floam
Copy link
Member

floam commented Oct 6, 2017

If you're OK with having to have to potentially track pip though, yeah, your way would make the first completion faster today.

@faho
Copy link
Member

faho commented Oct 6, 2017

Ah, okay.

Would it be possible to get the pip folks to just ship a completion file instead?

That would seem like the best of both worlds - skip the source step and track whatever pip is doing.

@floam
Copy link
Member

floam commented Oct 6, 2017

I guess it'd need to have some placeholder in there to be replaced by fish with the actual command name being used, since the same file could apply to pip, pip2, pip2.7.

@floam
Copy link
Member

floam commented Oct 6, 2017

Or multiple files. But sure, couldn't hurt to ask.

@floam
Copy link
Member

floam commented Oct 6, 2017

Now, the way python modules are installed, it might be an exercise actually locating a file installed into its share directory or wherever.

edit: nevermind, obviously we'd want them to have it install that into a system directory on install, just like the symlinks in /usr/local/bin.

@faho
Copy link
Member

faho commented Oct 6, 2017

Note: pip's documentation currently says to use

pip completion --fish > ~/.config/fish/completions/pip.fish

for fish. So I'd wager that, in practice, tracking of upstream changes doesn't happen anyway.

Now, I'd love to tell you how often the completion script changed but it changed files at the end of august and I can't get github to track that.

@floam
Copy link
Member

floam commented Oct 6, 2017

OK, I'm sold if you want to do it.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 6, 2017

What’s the suggestion again? Using a placeholder for the command name? I’m happy to do it

@faho
Copy link
Member

faho commented Oct 6, 2017

@cprieto: The suggestion is to embed the output of pip completion --fish directly into the completion script, instead of running pip completion --fish | source to get it.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 6, 2017

I am happy removing the pipX.fish files from the code and wait until everything looks fine in the pip side

@faho
Copy link
Member

faho commented Oct 6, 2017

Plus replacing the eval $COMP_WORDS[1] with pipX.

@cprieto
Copy link
Contributor Author

cprieto commented Oct 6, 2017

Ok I’m doing it, no problem

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants