-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
zsh support for native completions #3916
Comments
Had a few busy weeks but I'm back to working on this now. After trying to fit zsh support into the existing code, I realized that different shells might need to support a very different set of command line options for their completions and registration commands. This prompted me to split the complete command and the register command into different subcommands, with the shell as either a value (for registration) or as a subcommand that can define its own options for each shell. Following cobra's example, I've also prefixed the complete command with underscores to mark it as an internal command not for use by the consumer. Here's how usage would look now: # Write the bash completion script to stdout
dynamic completions bash -
# Process completions for bash (called from the completion script)
dynamic __complete bash --index=0 --type=9 -- a b c
# Write the zsh completion script to completions.zsh
dynamic completions zsh completions.zsh This results in a much better layout, where generic completion code can live inside Are you alright with changing the subcommand like this, or is there a strong requirement to keep the existing structure? (e.g. a single |
Could you go into more detail for as to why zsh needs something different? My biggest concern with splitting things out is ensuring users can add support for shells we don't officially support. The second is that the interface can have some complications to it and would prefer to have as much unified as possible to avoid duplicating problems. |
Sure thing! When trying to adapt the cobra zsh completion script, I noticed that no My solution would definitely lead to some code duplication. However, I feel like it would actually make it easier to implement support for new shells, since there's no chance of implementation details of one shell leaking out into others. When implementing a new shell, you get functions that will complete a given input, a framework for how to call the completion function, and you would just have to "fill in the middle part" that links the two. I could understand if you would prefer an abstraction that covers the largest amount of space to keep duplication to a minimum, though. |
Looks like
I'm fine changing how we express it or moving it into the bash layer. My overall hope was that the core completion logic would be shell agnostic and the registration code would bridge between clap and the shell as needed.
My concern for supporting other shells is the composability of the API. Since we don't have alternative shells, we don't have much for that today but its something we need to be able to handle as we do support more shells.
Isn't the function you described the |
Yes, my bad. I overlooked that, you're correct on that one.
Moving that into the completion registration script is absolutely an option. I'm just wary of a future where two sets of flags used by two different shells are completely disjunct. But I also concede that most shells we would implement right now have a large enough overlap to justify trying to make the code agnostic.
Yes, it is. The thing I was advocating for is essentially allowing each shell to define:
My hope would be that this would keep quirks of specific shells from leaking out to the completion code. Maybe I'm not doing a good job of describing what I'm proposing. I'll try to improve the model implementation I was working on and see how much duplication that would actually result in and what the resulting API would look like. That wouldn't be much work and then we'll actually have something tangible to discuss. |
I don't quite understand the role of |
I think that this could be a thing, because users may need the
_CLAP_IFS rather than IFS
|
See #3166 for more context
Tasks
The text was updated successfully, but these errors were encountered: