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.

Sign up for GitHub

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

Jump to bottom

Fix and test global completion in zsh #463

Merged
merged 4 commits into from
Nov 27, 2023
Merged

Fix and test global completion in zsh #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
215b2d8
Fix continuation chars in zsh
evanunderscore Nov 25, 2023
906a964
Fix completion in zsh special contexts
evanunderscore Nov 25, 2023
a0dc5e3
Remove pre-installed argcomplete on macos
evanunderscore Nov 25, 2023
8459dae
Re-enable skipped macOS tests
evanunderscore Nov 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ jobs:
run: |
[[ $(uname) == Linux ]] && sudo apt-get install --yes rpm tcsh fish zsh
[[ $(uname) == Darwin ]] && brew install bash tcsh fish
# Some runners have python-argcomplete preinstalled
# as a dependency of pipx, which interferes with the tests.
[[ $(uname) == Darwin ]] && brew uninstall --ignore-dependencies python-argcomplete || true
python -m pip install --quiet --upgrade codecov
- run: make install
- run: make lint
Expand Down
22 changes: 14 additions & 8 deletions argcomplete/bash_completion.d/_python-argcomplete
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,6 @@ _python_argcomplete_global() {
req_argv=( "" "${COMP_WORDS[@]:1}" )
__python_argcomplete_expand_tilde_by_ref executable
else
if [[ "$service" != "-default-" ]]; then
# TODO: this may not be sufficient - see https://zsh.sourceforge.io/Doc/Release/Completion-System.html
# May need to call _complete with avoid-completer=_python-argcomplete or something like that
_default
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kislyuk This was added in afdf75b, but I'm not sure what the intent was. This was causing default completions to be mixed in with the returned completions in my tests, and removing it doesn't seem to have caused any of the existing tests to fail. If you remember what problem this was addressing, I can take another look into it.

Copy link
Owner

Choose a reason for hiding this comment

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

@evanunderscore please do not delete this without a replacement, and especially please do not delete the comment. This was added in response to #454. Specifically, it was found that the default completer interferes with zsh completion in the redirect target context (words after > or 2>).

I didn't manage to write a test for this, and I since realized that this was a stopgap solution that will need to be further refined. For example, instead of calling _default unconditionally as a fallback here, we may need to call it only when certain contexts like -redirect- are listed in $service (see the link in the comment for a full list of contexts - there are others in the list that will almost certainly need similar treatment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the issue link, I did search initially but wasn't able to find it.

I've pushed a fix in 1f6e661. Rather than handle all of the special contexts, I've updated the compdef command to only override -default- which leaves the other special contexts undisturbed. The tests are all passing and I've poked around manually in the shell for a bit, and it seems to be working as expected.

Are you happy with this fix and the comment I've left?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately the macos tests are failing and I don't have a good way to troubleshoot them. I can just disable the global zsh tests on macos (since they weren't running at all before I added them), but it is strange that they were all passing before I made the compdef change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue is that some of the runners already have python-argcomplete installed as a dependency of pipx, which explains why the failures seemed completely inconsistent - the tests were actually testing whichever version happened to be installed and not what was in the source. For now, I've added a step to uninstall it, but a better solution would be to revisit the change I suggested in #255. Without it, there is no good way to avoid this mismatch.

Copy link
Owner

Choose a reason for hiding this comment

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

Rather than handle all of the special contexts, I've updated the compdef command to only override -default- which leaves the other special contexts undisturbed.

Sounds good, thanks.

return
fi
executable="${words[1]}"
req_argv=( "${words[@]:1}" )
fi
Expand Down Expand Up @@ -208,7 +202,15 @@ _python_argcomplete_global() {
_ARGCOMPLETE_SHELL="zsh" \
_ARGCOMPLETE_SUPPRESS_SPACE=1 \
__python_argcomplete_run "$executable" "${(@)req_argv[1, ${ARGCOMPLETE}-1]}"))
_describe "$executable" completions
local nosort=()
local nospace=()
if is-at-least 5.8; then
nosort=(-o nosort)
fi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The -o argument was removed in #433 due to incompatibility with zsh 5.7. The arguments to -o were added in 5.8:

Changes between 5.7 and 5.8
[...]
The compadd builtin's -o option now takes an optional argument to specify the order of completion matches. This affects the display of candidate matches and the order in which they are selected when cycling between them using menu completion.

https://zsh.sourceforge.io/releases.html

if [[ "${completions-}" =~ ([^\\]): && "${BASH_REMATCH[2]}" =~ [=/:] ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note this must use BASH_REMATCH (as opposed to match) because this script uses setopt local_options BASH_REMATCH. The indexing is different because we haven't also set KSH_ARRAYS and I haven't called __python_argcomplete_upshift_bash_rematch.

nospace=(-S '')
fi
_describe "$executable" completions "${nosort[@]}" "${nospace[@]}"
else
COMPREPLY=($(IFS="$IFS" \
COMP_LINE="$COMP_LINE" \
Expand All @@ -234,5 +236,9 @@ _python_argcomplete_global() {
if [[ -z "${ZSH_VERSION-}" ]]; then
complete -o default -o bashdefault -D -F _python_argcomplete_global
else
compdef _python_argcomplete_global -P '*'
autoload is-at-least
# Replace only the default completer (_default).
# There are many other special contexts we don't want to override.
# https://zsh.sourceforge.io/Doc/Release/Completion-System.html
compdef _python_argcomplete_global -default-
fi
11 changes: 10 additions & 1 deletion argcomplete/shell_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,15 @@
_ARGCOMPLETE_SHELL="zsh" \
_ARGCOMPLETE_SUPPRESS_SPACE=1 \
__python_argcomplete_run ${script:-${words[1]}}))
_describe "${words[1]}" completions -o nosort
local nosort=()
local nospace=()
if is-at-least 5.8; then
nosort=(-o nosort)
fi
if [[ "${completions-}" =~ ([^\\]): && "${match[1]}" =~ [=/:] ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note the different indexing semantics between match and BASH_REMATCH. Both versions are extracting the first match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The returned completion format is completion:description, where colons are escaped with backslashes. This looks for the character before the first unescaped colon and checks it against the same characters used for bash.

nospace=(-S '')
fi
_describe "${words[1]}" completions "${nosort[@]}" "${nospace[@]}"
else
local SUPPRESS_SPACE=0
if compopt +o nospace 2> /dev/null; then
Expand All @@ -67,6 +75,7 @@
if [[ -z "${ZSH_VERSION-}" ]]; then
complete %(complete_opts)s -F _python_argcomplete%(function_suffix)s %(executables)s
else
autoload is-at-least
compdef _python_argcomplete%(function_suffix)s %(executables)s
fi
"""
Expand Down
35 changes: 21 additions & 14 deletions test/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ def bash_repl(command="bash"):
def zsh_repl(command="zsh"):
sh = _repl_sh(command, ["--no-rcs", "-V"], non_printable_insert="%(!..)")
sh.run_command("autoload compinit; compinit -u")
# Require two tabs to print all options (some tests rely on this).
sh.run_command("setopt BASH_AUTO_LIST")
return sh


Expand Down Expand Up @@ -1256,9 +1258,6 @@ def setUp(self):
path = ":".join([os.path.join(BASE_DIR, "scripts"), TEST_DIR, "$PATH"])
sh.run_command("export PATH={0}".format(path))
sh.run_command("export PYTHONPATH={0}".format(BASE_DIR))
if self.repl_provider == bash_repl:
# Disable the "python" module provided by bash-completion
sh.run_command("complete -r python python2 python3")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not actually doing anything, since self.repl_provider is a separate method that calls bash_repl. Since the tests were still passing, I'm assuming this is no longer needed.

output = sh.run_command(self.install_cmd)
self.assertEqual(output, "")
# Register a dummy completion with an external argcomplete script
Expand Down Expand Up @@ -1313,18 +1312,24 @@ class TestZsh(TestBashZshBase, unittest.TestCase):
"test_parse_special_characters_dollar",
"test_comp_point", # FIXME
"test_completion_environment", # FIXME
"test_continuation", # FIXME
"test_wordbreak_chars", # FIXME
]

def repl_provider(self):
return zsh_repl()


@unittest.skipIf(BASH_MAJOR_VERSION < 4, "complete -D not supported")
class TestBashGlobal(TestBash):
class TestBashZshGlobalBase(TestBashZshBase):
install_cmd = 'eval "$(activate-global-python-argcomplete --dest=-)"'

def test_redirection_completion(self):
with TempDir(prefix="test_dir_py", dir="."):
self.sh.run_command("cd " + os.getcwd())
self.sh.run_command("echo failure > ./foo.txt")
self.sh.run_command("echo success > ./foo.\t")
with open("foo.txt") as f:
msg = f.read()
self.assertEqual(msg, "success\n")

def test_python_completion(self):
self.sh.run_command("cd " + TEST_DIR)
self.assertEqual(self.sh.run_command("python3 ./prog basic f\t"), "foo\r\n")
Expand Down Expand Up @@ -1364,9 +1369,6 @@ def _test_console_script(self, package=False, wheel=False):
command = "pip install {} --target .".format(test_package)
if not wheel:
command += " --no-binary :all:"
if sys.platform == "darwin":
# Work around https://stackoverflow.com/questions/24257803
command += ' --install-option="--prefix="'
install_output = self.sh.run_command(command)
self.assertEqual(self.sh.run_command("echo $?"), "0\r\n", install_output)
command = "test-module"
Expand All @@ -1375,27 +1377,32 @@ def _test_console_script(self, package=False, wheel=False):
command += " a\t"
self.assertEqual(self.sh.run_command(command), "arg\r\n")

@unittest.skipIf(os.uname()[0] == "Darwin", "Skip test that fails on MacOS")
kislyuk marked this conversation as resolved.
Show resolved Hide resolved
def test_console_script_module(self):
"""Test completing a console_script for a module."""
self._test_console_script()

@unittest.skipIf(os.uname()[0] == "Darwin", "Skip test that fails on MacOS")
def test_console_script_package(self):
"""Test completing a console_script for a package."""
self._test_console_script(package=True)

@unittest.skipIf(os.uname()[0] == "Darwin", "Skip test that fails on MacOS")
def test_console_script_module_wheel(self):
"""Test completing a console_script for a module from a wheel."""
self._test_console_script(wheel=True)

@unittest.skipIf(os.uname()[0] == "Darwin", "Skip test that fails on MacOS")
def test_console_script_package_wheel(self):
"""Test completing a console_script for a package from a wheel."""
self._test_console_script(package=True, wheel=True)


@unittest.skipIf(BASH_MAJOR_VERSION < 4, "complete -D not supported")
class TestBashGlobal(TestBash, TestBashZshGlobalBase):
pass


class TestZshGlobal(TestZsh, TestBashZshGlobalBase):
pass


class Shell:
def __init__(self, shell):
self.child = pexpect.spawn(shell, encoding="utf-8")
Expand Down