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(complete)!: Allow alternative shells for dynamic completions #5018

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

epage
Copy link
Member

@epage epage commented Jul 19, 2023

This covers the API changes from #4177.

This switches the communication from the registration script to the completion code from flags to env variables. This is less helpful for hand-experimenting which is a minority case. This does mean we don't have to try to abstract every single shell and allows easier evolution since we don't have to spend time bike shedding what the contract should be.

@epage epage mentioned this pull request Jul 19, 2023
@ModProg
Copy link
Contributor

ModProg commented Jul 19, 2023

Moving them to environment args looks like a good idea.

I'm not sure about the generic complete(...).

For implementing fish I'd need to differentiate between flags and values starting with -.
I'd also want to be able to, instead of providing the current dir, provide the completions, as fish's directory completion is a bit more capable than what clap has.

This is why I proposed the completion context, to allow moving this logic into the shell specific code:
d820175#diff-8f996293080d41d5fafd69b78421dddba0b5e44fd3e43962bf906369602a8362R125-R400

@epage
Copy link
Member Author

epage commented Jul 19, 2023

I think we can re-evaluate and evolve this as we go. We don't have to support every case for every shell up front. For example complete returns a Vec<OsString> when we'll likely want Vec<OsString, StyledStr> or something even more.

I will say that we will not be having per-shell parsers; that would go counter to what this is trying to accomplish. If there is a policy we want, like not showing flags for new completions, then we should consider applying that universally. If we really need some level of customization for the current field, we can try to decouple that from parsing but that is adding complexity I'd like to avoid.

@epage epage merged commit 0951f93 into clap-rs:master Jul 19, 2023
21 checks passed
@epage epage deleted the dynamiite branch July 19, 2023 14:09
@ModProg
Copy link
Contributor

ModProg commented Jul 19, 2023

yeah, the CompletionContext I did in my PR would also have been supposed to be applied to all shells.

It was more about what is the interface between the two, my Idea was to decouple parsing and the actual strings outputted by the completion more, i.e. the parser wouldn't directly return strings with possible completions but something with more detail, something that can differentiate what kind of completion item something is, i.e. flag, value, etc.

But I also see that we could just try to have the most capable completions regardless of shell, i.e. pick for everything the shell providing the best1 features and implement that for all shells.

Footnotes

  1. this is obviously subjective and might need to be configurable, i.e. maybe someone uses bash because they dont want their completions to join short completions.

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

2 participants