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

feat(zsh): Add default completion as fallback #2331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wkunert
Copy link

@wkunert wkunert commented May 9, 2023

Fallback to default completion if no match was found (see https://zsh.sourceforge.io/Doc/Release/Completion-System.html#Completion-Functions for details on _default).

Addresses #2113 and harmonizes the behavior of zsh/bash completion, see -o default in the bash completion template:

complete -o bashdefault -o default -F _{{app_name}}_yargs_completions {{app_name}}

@shadowspawn
Copy link
Member

This is simple, but I am not sure it is correct. I am researching this to review the PR, and not an expert!

I don't see in that documentation any mention of the result code of calling _describe, but I think you are relying on getting a non-zero result if reply is empty? Have you seen this pattern used anywhere else? The documentation suggests it might be used in an error condition which I don't think is quite the case here.

There are a huge number of utility functions! I expect there is support for considering multiple or fallback targets somewhere in there... (Styles? Tags? Other functions? So many possibilities!)

For instance, I found howto documentation covering _describe (simpler) vs _alternative (more complex):

@wkunert
Copy link
Author

wkunert commented May 14, 2023

Thanks for taking the time @shadowspawn to look into it!

This is simple, but I am not sure it is correct. I am researching this to review the PR, and not an expert!

I am not an expert either, the missing file completion was just bugging me enough to give it a shot. 😄

I don't see in that documentation any mention of the result code of calling _describe, but I think you are relying on getting a non-zero result if reply is empty? Have you seen this pattern used anywhere else? The documentation suggests it might be used in an error condition which I don't think is quite the case here.

I am sure to have seen it somewhere but I am not able to find it again and I also cannot find any documentation on it. It does work but it might be just a quirk. I went ahead and changed the code to make it more explicit. The docs are mentioning "completion after an unrecognised subcommand" as an error condition which I thought does fit here.

There are a huge number of utility functions! I expect there is support for considering multiple or fallback targets somewhere in there... (Styles? Tags? Other functions? So many possibilities!)

I agree that there might be better solutions but it becomes really complicated very quickly. For now I will stick with my simple approach which is sufficient for my use case.

Thanks again for your time and pointing out this flaw, I hope the change clears your concern.

@shadowspawn
Copy link
Member

I am happier checking reply itself. (Although at some point I'll have to investigate when this triggers and if I agree it is appropriate!)

What does reply contain? A string or an array?

If array, this looks a bit strange, and does not work when I try it locally:

$ array=(one two three)
$ echo \${#array[@]} 
zsh: no matches found: ${#array[@]}

I think the array length check might look like:

$ array=(one two three)
$ echo ${#array} 

3

Using double brackets also gives safer test behaviour, so line might be:

if [[ ${#reply} -gt 0 ]]; then

The difference between [ condition ] and [[ condition ]] is subtle. I'll leave it to references:

Fallback to default completion if no match was found (see
https://zsh.sourceforge.io/Doc/Release/Completion-System.html#Completion-Functions
for details on `_default`).
@wkunert
Copy link
Author

wkunert commented May 14, 2023

Thanks again for your time and input @shadowspawn!

What does reply contain? A string or an array?

As far as I see it, reply should be an array because of the parentheses reply=(…):

IFS=$'\n' reply=($(COMP_CWORD="$((CURRENT-1))" COMP_LINE="$BUFFER" COMP_POINT="$CURSOR" {{app_path}} --get-yargs-completions "\${words[@]}"))

If array, this looks a bit strange, and does not work when I try it locally:

$ array=(one two three)
$ echo \${#array[@]} 
zsh: no matches found: ${#array[@]}

Did you by any chance keep the backslash? This is only in the template because we are within an (JavaScript) template literal and therefore the dollar sign must be escaped. For me (without the backslash) it looks like this in the shell:

% array=(one two three)
% echo ${#array[@]}    
3

I think the array length check might look like:

$ array=(one two three)
$ echo ${#array} 

3

As far as I understand the zsh documentation both versions can be used interchangeably (and there is even a third version: ${#array[*]}), but I changed it to the more straightforward style you proposed.

Using double brackets also gives safer test behaviour, …

Agree, changed accordingly. Thanks for the references!

@shadowspawn
Copy link
Member

shadowspawn commented May 14, 2023

I missed being inside (JavaScript) template literal! Apologies. I was just looking at the diff and saw all shell code...

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

I gave this a light test comparing zsh and bash completions, and LGTM.

@alyssadev
Copy link

Hi, please consider pulling this PR, thank you

@Goli4thus
Copy link

I just stumbled over this PR while searching for a way to make zsh completion work, when I have an option that expect a filepath value.

I simply patched my ~/.zshrc completion section generated today via yargs according to this PR.
But that at first gave me errors (three times the same one):

bad math expression: illegal character: {

I then removed the leading backslash in \${#reply}.
EDIT: After actually skimming this PR's discussion it's clear now: was from javascript templating.

Result is: then it works nicely!

All that's needed (after a refresh; e.g. via exec zsh) is to force it to switch to the default completion.
That can be done easily via ./<TAB>, ~/<TAB>, or (if the desired file/folder-name is known roughly) by writing first letter (or more) and then <TAB>.

So this PR certainly seems a nice addition to make zsh completion work regarding filepath completion.

Additional infos:
Tested on manjaro linux with zsh version 5.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants