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

Support dynamic value of argument completion #5621

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

shannmu
Copy link
Contributor

@shannmu shannmu commented Aug 2, 2024

Related issue #1232

Work in this PR

  • Add CustomCompleter trait and ArgValueCompleter struct to provide users a way to add custom value of argument completion.
  • Add test case to show the feat.

What may be left

Open question: does the completion function accept just the arg and current value or does it have access to ArgMatches for completing based on other data like in #1910

  • Is there any way to access ArgMatches before the builder of the Command. We add custom value hint in builder now. If we really need this feat, we may need to do more things.

@shannmu
Copy link
Contributor Author

shannmu commented Aug 2, 2024

Thanks for your review! Do you think it is necessary to prevent the spread of user code errors? I think it may be necessary, because the completion code is executed implicitly, and if the spread of errors is not prevented, it may cause confusion to users. Also, I think my current implementation is not good, maybe we can use other rust tools or crates to do this.

@epage
Copy link
Member

epage commented Aug 2, 2024

I think it may be necessary, because the completion code is executed implicitly, and if the spread of errors is not prevented, it may cause confusion to user

If the user wants panic prevention, they can provide it. I don't think we need to be doing it for them

Also, I think my current implementation is not good, maybe we can use other rust tools or crates to do this.

Which parts are you concerned about? Do you have something in mind?

@shannmu
Copy link
Contributor Author

shannmu commented Aug 5, 2024

Which parts are you concerned about? Do you have something in mind?

I mean the panic prevention part. I want to let the user code execute in a sandbox so as not to affect other possible completions.

@shannmu shannmu force-pushed the dynamic_valuehint branch 3 times, most recently from a2534e4 to 0ff617a Compare August 7, 2024 09:15
clap_complete/Cargo.toml Outdated Show resolved Hide resolved
@shannmu shannmu force-pushed the dynamic_valuehint branch from 0ff617a to 77c3efc Compare August 7, 2024 17:30
@shannmu shannmu marked this pull request as ready for review August 7, 2024 17:39
@shannmu shannmu force-pushed the dynamic_valuehint branch from 77c3efc to 9410401 Compare August 7, 2024 19:33
clap_complete/src/dynamic/completer.rs Outdated Show resolved Hide resolved
clap_complete/src/dynamic/completer.rs Outdated Show resolved Hide resolved
clap_complete/src/dynamic/completer.rs Outdated Show resolved Hide resolved
@shannmu shannmu force-pushed the dynamic_valuehint branch from 9410401 to fd5f2fa Compare August 7, 2024 19:50
@shannmu shannmu force-pushed the dynamic_valuehint branch from fd5f2fa to 0dd7cdc Compare August 8, 2024 12:59
@shannmu
Copy link
Contributor Author

shannmu commented Aug 8, 2024

ArgExt has a restriction requiring the Debug trait, which means closures cannot be directly used (as closures cannot implement the Debug trait). Therefore, I added a wrapper called ClosureCompleter that allows users to customize the debug info. I'm not sure if this is the best approach, so if you have any other ideas, please let me know.

@shannmu shannmu force-pushed the dynamic_valuehint branch from 0dd7cdc to 3239c02 Compare August 8, 2024 15:12
@shannmu shannmu force-pushed the dynamic_valuehint branch from 3239c02 to 7fd7b3e Compare August 8, 2024 15:47
@epage epage enabled auto-merge August 8, 2024 15:52
@epage epage merged commit 2690e1b into clap-rs:master Aug 8, 2024
24 of 25 checks passed
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