Fix and test global completion in zsh #463

evanunderscore · 2023-11-25T07:32:44Z

codecov-commenter · 2023-11-25T07:35:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9862df4) 81.12% compared to head (8459dae) 81.12%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #463   +/-   ##
========================================
  Coverage    81.12%   81.12%           
========================================
  Files           10       10           
  Lines          784      784           
========================================
  Hits           636      636           
  Misses         148      148

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

evanunderscore · 2023-11-25T07:34:50Z

argcomplete/bash_completion.d/_python-argcomplete

@@ -139,9 +139,6 @@ _python_argcomplete_global() {
        __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


@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.

@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).

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?

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.

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.

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.

evanunderscore · 2023-11-25T07:37:02Z

argcomplete/bash_completion.d/_python-argcomplete

+            if is-at-least 5.8; then
+                nosort=(-o nosort)
+            fi
+            if [[ "${completions-}" =~ ([^\\]): && "${BASH_REMATCH[2]}" =~ [=/:] ]]; then


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.

evanunderscore · 2023-11-25T07:38:14Z

argcomplete/bash_completion.d/_python-argcomplete

+            local nospace=()
+            if is-at-least 5.8; then
+                nosort=(-o nosort)
+            fi


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

evanunderscore · 2023-11-25T07:40:07Z

argcomplete/shell_integration.py

+        if is-at-least 5.8; then
+            nosort=(-o nosort)
+        fi
+        if [[ "${completions-}" =~ ([^\\]): && "${match[1]}" =~ [=/:] ]]; then


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

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.

evanunderscore · 2023-11-25T07:40:53Z

test/test.py

@@ -1248,9 +1248,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")


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.

The linked workaround seems no longer relevant. The Homebrew documentation no longer mentions the flag.

test/test.py

kislyuk · 2023-11-27T10:33:28Z

Thanks @evanunderscore. I'll do some more testing with the new compdef change and then roll a release.

kislyuk · 2023-12-03T01:05:14Z

@evanunderscore after this change, global completion doesn't work in zsh for me, on both Linux and macos. How did you test it?

kislyuk · 2023-12-03T01:27:22Z

@evanunderscore I have reverted the part of this PR that changes the zsh compdef line (and re-added the call to _default).

It doesn't look like compdef -default- will work for global completion.

kislyuk · 2023-12-03T01:28:59Z

Actually, since the revert of just that part breaks tests, I'm going to go ahead and revert the whole PR; please reopen it when you have a chance and see if your changes can be reconciled with a version of the compdef line that works.

This reverts commit cb5fc38.

kislyuk · 2023-12-03T02:28:35Z

OK, I think I found a way to preserve the behavior that we want - I've replaced -default- with -first in the compdef line (but note the autoload definition line, the first line of the file, remains #compdef -P *, which is also key). However the tests are now failing, can you please take a look?

In light of the apparent fragility of this change, it will require further testing and hopefully we can find a way to exercise it in CI. Note that it can be tricky to make sure that zsh unloaded the previous function and loaded the one that you edited - I had to remove ~/.zcompdump sometimes to make the changes effective.

evanunderscore · 2023-12-07T23:31:07Z

@kislyuk The tests are failing because you've reintroduced the problem from #458 by not suppressing the default completions. As an example, test_wordbreak_chars is expecting this:

% prog break a<tab>
% prog break a:b:<user types c>

The completion of :b: is unambiguous between the two possible completions, so is inserted when the user presses tab. However if default completion is not suppressed, there is a third option:

% prog break a<tab>
Completing test/prog
a:b:d  a:b:c  --                                                              
Completing file
argcomplete/
% prog break a<user types c>

Now it doesn't insert anything, and the resulting line is just ac, hence the error message.

I agree it's not ideal for unrelated tests to fail like this. The fix would be to have the shell tests cd into an empty directory so that filenames can't incidentally ruin tests, but we would have to add another test that intentionally tries to trigger this problem so we don't end up with another regression for #458.

@evanunderscore after this change, global completion doesn't work in zsh for me, on both Linux and macos. How did you test it?

I tested on Ubuntu 22.04 with zsh 5.8.1 using the same method as the tests use, i.e. 'eval "$(activate-global-python-argcomplete --dest=-)"'. How did you test it? What happened when you tested it, e.g. did completion break entirely or was it only argcomplete completions that did not work? Did you get any useful output when you set _ARC_DEBUG=1?

In light of the apparent fragility of this change, it will require further testing and hopefully we can find a way to exercise it in CI. Note that it can be tricky to make sure that zsh unloaded the previous function and loaded the one that you edited - I had to remove ~/.zcompdump sometimes to make the changes effective.

To the best of my understanding, the change was being exercised in CI. The problem will have to be either with a mismatch between how you and I tested locally, or the version of zsh you're using, or something else specific to your environment.

evanunderscore · 2023-12-08T00:00:39Z

OK, I think I found a way to preserve the behavior that we want - I've replaced -default- with -first in the compdef line (but note the autoload definition line, the first line of the file, remains #compdef -P *, which is also key). However the tests are now failing, can you please take a look?

The #compdef -P * is the part that I missed - I didn't realise there was any significance to it (and I don't think there is when the file is run directly). For reference, it's documented under "20.2.2 Autoloaded files" here: https://zsh.sourceforge.io/Doc/Release/Completion-System.html

Changing this to #compdef -default- to match the compdef line seems to fix it for me when installing globally. I'm not totally sure I understand how that comment and the actual compdef command interact, or if we're using them correctly.

Can you try reverting your fix and changing the #compdef comment please?

evanunderscore commented Nov 25, 2023

View reviewed changes

evanunderscore force-pushed the cont-chars branch 2 times, most recently from 7caa780 to b6e38aa Compare November 25, 2023 08:14

evanunderscore added 3 commits November 26, 2023 12:31

Fix continuation chars in zsh

215b2d8

Fix completion in zsh special contexts

906a964

Remove pre-installed argcomplete on macos

a0dc5e3

evanunderscore force-pushed the cont-chars branch from 1f6e661 to a0dc5e3 Compare November 26, 2023 01:32

Re-enable skipped macOS tests

8459dae

The linked workaround seems no longer relevant. The Homebrew documentation no longer mentions the flag.

evanunderscore commented Nov 26, 2023

View reviewed changes

test/test.py Show resolved Hide resolved

kislyuk merged commit cb5fc38 into kislyuk:develop Nov 27, 2023
22 checks passed

This was referenced Nov 27, 2023

installing zsh globally adds paths to completion options introduced in v3.1.4 #458

Closed

How do I exclude files in the users directory from the tab completion? #462

Closed

kislyuk added a commit that referenced this pull request Dec 3, 2023

Revert "Fix and test global completion in zsh (#463)"

d696194

This reverts commit cb5fc38.

This was referenced Dec 8, 2023

Fix zsh global install #466

Merged

How can I suppress unnecessary variables outside of my program? #442

Open

Allow explicit zsh global completion activation #467

Merged

kislyuk mentioned this pull request Jun 18, 2024

Consider using completer declaration other than "-default-" for zsh global completion #491

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and test global completion in zsh #463

Fix and test global completion in zsh #463

evanunderscore commented Nov 25, 2023

codecov-commenter commented Nov 25, 2023 •

edited

Loading

evanunderscore Nov 25, 2023

kislyuk Nov 25, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 26, 2023

kislyuk Nov 27, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 25, 2023

evanunderscore Nov 25, 2023

kislyuk commented Nov 27, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

evanunderscore commented Dec 7, 2023

evanunderscore commented Dec 8, 2023

Fix and test global completion in zsh #463

Fix and test global completion in zsh #463

Conversation

evanunderscore commented Nov 25, 2023

codecov-commenter commented Nov 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kislyuk commented Nov 27, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

kislyuk commented Dec 3, 2023

evanunderscore commented Dec 7, 2023

evanunderscore commented Dec 8, 2023

codecov-commenter commented Nov 25, 2023 •

edited

Loading