kislyuk / argcomplete

Python and tab completion, better together.
https://kislyuk.github.io/argcomplete/
Apache License 2.0
1.39k stars 129 forks source link

Fix and test global completion in zsh #463

Closed evanunderscore closed 7 months ago

evanunderscore commented 7 months ago

Fixes #451, #458, #462

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #463 +/- ## ======================================== Coverage 81.12% 81.12% ======================================== Files 10 10 Lines 784 784 ======================================== Hits 636 636 Misses 148 148 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kislyuk commented 7 months ago

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

kislyuk commented 7 months ago

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

kislyuk commented 7 months ago

@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 commented 7 months ago

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.

kislyuk commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

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?