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

fix(zsh): Use _default as the completion fallback where the ValueHint is Unknown #5791

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

okapia
Copy link

@okapia okapia commented Oct 28, 2024

This is consistent with bash where compgen -f is used in such cases. In long experience with completions distributed with zsh, the worst thing you can do is break filename completion as that's the minimum most user's expect. By default, _default just completes files. It is also far better to leave the final component blank from ValueHint::Other as using empty parentheses will cause zsh to do slightly more work for the same result.

For context, note that I'm a zsh developer and have a lot of experience of writing zsh completions manually.

That is for a ValueHint of Unknown. This is consistent with bash where
compgen -f is used in such cases. In long experience with completions
distributed with zsh, the worst thing you can do is break filename
completion as that's the minimum most user's expect.
@epage
Copy link
Member

epage commented Oct 28, 2024

Thanks!

For context, note that I'm a zsh developer and have a lot of experience of writing zsh completions manually.

btw have you seen our new completion system which moves most of the runtime logic to Rust (#3166)? We got descriptions working in #5775 and the author of that has an idea on how to fix #5752. If you have any zsh-specific insights for how to improve it further more generally or for mimicking any of the carapace features (#5651, #5650, #5649), that would be great! I only use bash, am not a heavy completion user, and find the documentation for these things only focused on general hand maintained completions, making it difficult for me to help improve some of the shell integration.

@epage
Copy link
Member

epage commented Oct 28, 2024

Looks like you need to run

$ SNAPSHOTS=overwrite cargo test -p clap_complete  -F unstable-shell-tests

@okapia
Copy link
Author

okapia commented Oct 28, 2024

btw have you seen our new completion system which moves most of the runtime logic to Rust (#3166)?

So if I understand, this is taking the approach of running something like:

COMPREPLY=( $("my-app" complete --index ${COMP_CWORD} --type ${COMP_TYPE} ${SPACE_ARG} --ifs="$IFS" -- "${COMP_WORDS[@]}") )

And doing all the logic in Rust code. This approach can be tempting when you consider only bash's rather limited interface for completion but it ends up dumbing down the functionality of any other shell to the level of bash. Allowing match:description seems to be the approach to supporting descriptions but I can think of far more features besides that won't be covered. And duplicating half of the functionality of a half-dozen shells in Rust for every application is adding a lot of bloat. It is better to leave matching words to the command-line, handling quoting, nested quoting, prefixes and suffixes, approximate, case-insensitive or partial matching to the shell. Generating functions that can be installed can be the best way to ensure completions are enabled for end users without additional manual effort. And completions are needed for something special like the cargo targets, it is better to add a very basic simple option for listing those targets - that can then be used not just for completion but for many other things.

As to #5752, the short answer, is to use the -o nosort option to compadd and define a group for the matches. But a decent completion would put options and targets in separate groups at which point the sorting would not be a problem. Zsh allows matches to be grouped. The -o nosort option makes any particular group unsorted which should only be used if there is a particular reason such as ordering something like "seconds, minutes, hours, days" where alphabetic ordering is less logical than ascending or descending order of duration. To support older releases of zsh, -V is needed to define unsorted groups instead of -J. That still works in newer zsh but the -o option has other options like numeric ordering.

@okapia
Copy link
Author

okapia commented Oct 28, 2024

Looks like you need to run

$ SNAPSHOTS=overwrite cargo test -p clap_complete  -F unstable-shell-tests

That fails for me on current master with error[E0658]: use of unstable library feature 'split_at_checked. I'll try upgrading rustc.

@epage epage merged commit 6ec0b43 into clap-rs:master Oct 29, 2024
25 checks passed
@epage
Copy link
Member

epage commented Oct 29, 2024

So if I understand, this is taking the approach of running something like:

Something to that effect.

The plan is to deprecate the existing completions in clap_complete and eventually move them out to a lower tier of support in different packages unless we have a clear understanding of the problem.

Our issue tracking this is #3166 and the issue specifically for parity in zsh with the existing completions is #3916. If you are concerned about this plan, I would recommend opening issues about the missing features and incorrect behavior so we have a more concrete idea to go off of than high level concerns.

This is coming about because we've found the existing approach of generating (or hand maintaining) completions for each shell isn't scalable. It requires expertise in a shell to implement or review, it is hard to test, we've had a plethora of issues building up in supporting each shell, we've had a wide disparity in offered features for each shell because implementers aren't generally skilled in every shell, etc. So far, we've only needed minimal shims in the shell language and Rust for each shell while we're able to offer a smarter set of completions that is tailored to the extra information clap has that the shells do not. We're not the only ones doing this as there are cases of CLI parsers doing this in Rust, Python, Go, etc.

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.

2 participants