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

Add zsh completion #11409

Closed
wants to merge 1 commit into from
Closed

Add zsh completion #11409

wants to merge 1 commit into from

Conversation

Freed-Wu
Copy link
Contributor

@Freed-Wu Freed-Wu commented Aug 26, 2022

Generate zsh completion script by python

Fix #4955
Fix #5364
Fix https://bugs.archlinux.org/task/65349#comment187166
Remove https://github.com/zsh-users/zsh/blob/master/Completion/Unix/Command/_pip

And the old pip completion --zsh can be removed.

❯ pip -<TAB>
option
--cache-dir                   Store the cache data in <dir>.
--cert                        Path to PEM-encoded CA certificate bundle. If provided, overrides the default. See 'SSL Certificate Verification' in pip documentation for more information.
--client-cert                 Path to SSL client certificate, a single file containing the private key and the certificate in PEM format.
...
❯ pip <TAB>
command
cache        Inspect and manage pip's wheel cache.
check        Verify installed packages have compatible dependencies.
...
❯ pip install -e <TAB>
file
URL prefix
AUTHORS.txt     file:           gopher://       https://        MANIFEST.in     NEWS.rst        pyproject.toml  setup.cfg       src/            tools/
docs/           ftp://          http://         LICENSE.txt     news/           noxfile.py      README.rst      setup.py        tests/
❯ pip install <TAB>
zsh: do you wish to see all 395333 possibilities (395335 lines)?
❯ pip install --<TAB>
option
package
--abi                         Only use wheels compatible with Python abi <abi>, e.g. 'pypy_41'. If not specified, then the current interpreter abi tag is used. Use this option multiple times to specify multiple abis supported by the target interpreter. Generally you will need to specify --implementation, --platform, and --python-version when using this option.
--cache-dir                   Store the cache data in <dir>.
--cert                        Path to PEM-encoded CA certificate bundle. If provided, overrides the default. See 'SSL Certificate Verification' in pip documentation for more information.
...
❯ pip download --implementation <TAB>
implementation
cp   cpython
ip   ironpython
jy   jython
pp   pypy
py   implementation-agnostic

Comment on lines 547 to 555
choices=[
"any",
"manylinux1_x86_64",
"manylinux1_i386",
"manylinux2014_aarch64",
"win_amd64",
"macosx_10_9_x86_64",
"macosx_11_0_arm64",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any resource to list all platforms? I just search some one, but it is not all.

And some checks are fail because of similar reason. Such as --implementation fakepy. fakepy seems not to be one of implementation.

@@ -300,18 +300,21 @@ class PipOption(Option):
)


exists_actions = ["switch", "ignore", "wipe", "backup", "abort"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exists_actions = ["switch", "ignore", "wipe", "backup", "abort"]
EXISTS_ACTIONS = {
"s": "switch",
"i": "ignore",
"w": "wipe",
"b": "backup",
"a": "abort",
}

This maintains the readability, while eliminating the need to add map(lambda x: x[0]) everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I support this thought. It is more readable.

@Freed-Wu
Copy link
Contributor Author

Only one test failed. What does it means?

tests/functional/test_truststore.py::test_truststore_error_without_preinstalled 
[gw1] [ 72%] FAILED tests/functional/test_requests.py::test_timeout 

default=[],
action="append",
metavar="action",
help="Default action when a path already exists: "
"(s)witch, (i)gnore, (w)ipe, (b)ackup, (a)bort.",
", ".join(EXISTS_ACTIONS.values()),
Copy link
Member

Choose a reason for hiding this comment

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

This changes the output from:

(s)witch, (i)gnore, (w)ipe, (b)ackup, (a)bort

to...

switch, ignore, wipe, backup, abort

Given that we don't take "switch" but rather "s" as input here, I'd strongly prefer that we preserve the existing style.

@pradyunsg
Copy link
Member

pradyunsg commented Aug 27, 2022

Thanks for filing this PR! I have a few questions:

  • Is it not possible to implement the new pieces of functionality here, as a part of the function called by pip completion --zsh? That way, these improvements won't be limited to just zsh and would keep the logic in Python (where it's easier to test as well).

    def autocomplete():

    This is the documented mechanism: https://pip.pypa.io/en/stable/user_guide/#command-completion

  • I see that there's a template involved. Is the zsh completion script supposed to be regenerated as a part of the release?


[gw1] [ 72%] FAILED tests/functional/test_requests.py::test_timeout

That's fine -- you can ignore that one, it's a flaky test. :/

@Freed-Wu
Copy link
Contributor Author

Is it not possible to implement the new pieces of functionality here, as a part of the function called by pip completion --zsh? That way, these improvements won't be limited to just zsh and would keep the logic in Python (where it's easier to test as well).

I know a method: https://github.com/iterative/shtab, it support bash/zsh/tcsh.
Its method is same as this PR -- use parser._acitons to generate complete
script from shell template. Its usage is:

❯ pip --print-completion bash|sudo tee /usr/share/bash-completion/completions/pip
❯ pip --print-completion zsh|sudo tee /usr/share/zsh/site-functions/_pip
❯ pip --print-completion tcsh|sudo tee /etc/profile.d/pip.completion.csh

However, it only support argparse, not optparse. So before pip support argparse
or shtab support optparse or some library support converting
optparse.OptionParser to argparse.ArgumentParser, this PR can do a temporary
job. At least it can replace
https://github.com/zsh-users/zsh/blob/master/Completion/Unix/Command/_pip and
fix https://bugs.archlinux.org/task/65349#comment187166, #4955, #5364.

Other methods: click has a different syntax from argparse/optparse,
argcomplete/pyzshcomplete is slow. (see https://docs.iterative.ai/shtab/#alternatives)

I see that there's a template involved. Is the zsh completion script supposed
to be regenerated as a part of the release?

Right!

That's fine -- you can ignore that one, it's a flaky test. :/

Thanks 😄

@pradyunsg
Copy link
Member

pradyunsg commented Aug 28, 2022

I know a method

We don’t need an external dependency for this. pip already has logic for providing shell completion from Python, by looking at its own CLI setup:

def autocomplete():
"""Command and option completion for the main option parser (and options)
and its subcommands (and options).
Enable by sourcing one of the completion shell scripts (bash, zsh or fish).
"""
# Don't complete if user hasn't sourced bash_completion file.
if 'PIP_AUTO_COMPLETE' not in os.environ:
return
cwords = os.environ['COMP_WORDS'].split()[1:]
cword = int(os.environ['COMP_CWORD'])
try:
current = cwords[cword - 1]
except IndexError:
current = ''
subcommands = [cmd for cmd, summary in get_summaries()]
options = []
# subcommand
try:
subcommand_name = [w for w in cwords if w in subcommands][0]
except IndexError:
subcommand_name = None
parser = create_main_parser()
# subcommand options
if subcommand_name:
# special case: 'help' subcommand has no options
if subcommand_name == 'help':
sys.exit(1)
# special case: list locally installed dists for show and uninstall
should_list_installed = (
subcommand_name in ['show', 'uninstall'] and
not current.startswith('-')
)
if should_list_installed:
installed = []
lc = current.lower()
for dist in get_installed_distributions(local_only=True):
if dist.key.startswith(lc) and dist.key not in cwords[1:]:
installed.append(dist.key)
# if there are no dists installed, fall back to option completion
if installed:
for dist in installed:
print(dist)
sys.exit(1)
subcommand = commands_dict[subcommand_name]()
for opt in subcommand.parser.option_list_all:
if opt.help != optparse.SUPPRESS_HELP:
for opt_str in opt._long_opts + opt._short_opts:
options.append((opt_str, opt.nargs))
# filter out previously specified options from available options
prev_opts = [x.split('=')[0] for x in cwords[1:cword - 1]]
options = [(x, v) for (x, v) in options if x not in prev_opts]
# filter options by current input
options = [(k, v) for k, v in options if k.startswith(current)]
for option in options:
opt_label = option[0]
# append '=' to options which require args
if option[1] and option[0][:2] == "--":
opt_label += '='
print(opt_label)
else:
# show main parser options only when necessary
if current.startswith('-') or current.startswith('--'):
opts = [i.option_list for i in parser.option_groups]
opts.append(parser.option_list)
opts = (o for it in opts for o in it)
for opt in opts:
if opt.help != optparse.SUPPRESS_HELP:
subcommands += opt._long_opts + opt._short_opts
print(' '.join([x for x in subcommands if x.startswith(current)]))
sys.exit(1)

My question is: Why should we add a separate shell script, instead of extending the existing Python code we have for completions?

Right!

Ok, that’s slightly problematic. Our release processes are automated and, ideally, this regeneration is covered as a part of that. I don’t think doing code generation is a good idea though, since we don’t have any automated tests for this generated script, so we won’t know when it’s broken after being regenerated outside of testing it manually, something that increases the amount of work we need to do during a release.

We already have a mechanism to provide completions to zsh users, and I’d prefer that we improve it instead of replacing it with something that, while it’s better, represents greater maintenance burden, increases risks of breakage during a release and reduces the likelihood that we’d be able to test this in a portable manner.

@Freed-Wu
Copy link
Contributor Author

Freed-Wu commented Aug 28, 2022

We're planning on switching the CLI to click or argparse in the future, instead of pip's homegrown solution.

We don’t need an external dependency for this. pip already has logic for providing shell completion from Python, by looking at its own CLI setup:

It seems that the original project is to switch external schedule to give up pip's homegrown solution. If so, I think this PR can provide a temporary and enough available solution before this switch.

Now the project has changed?

And some scheme like shtab is very small, I think it even can be copied to /pip/_shtab.py as an internal module.

@Freed-Wu Freed-Wu closed this Nov 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants