Skip to content

Support dynamic value of argument completion#5621

Merged
epage merged 3 commits intoclap-rs:masterfrom
shannmu:dynamic_valuehint
Aug 8, 2024
Merged

Support dynamic value of argument completion#5621
epage merged 3 commits intoclap-rs:masterfrom
shannmu:dynamic_valuehint

Conversation

@shannmu
Copy link
Copy Markdown
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.

Comment thread clap_builder/src/builder/value_hint.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
Comment thread clap_builder/src/builder/value_hint.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
@shannmu
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread clap_complete/Cargo.toml Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
Comment thread clap_complete/tests/testsuite/dynamic.rs Outdated
@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
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
@shannmu shannmu force-pushed the dynamic_valuehint branch from 9410401 to fd5f2fa Compare August 7, 2024 19:50
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
Comment thread clap_complete/src/dynamic/completer.rs Outdated
@shannmu shannmu force-pushed the dynamic_valuehint branch from fd5f2fa to 0dd7cdc Compare August 8, 2024 12:59
@shannmu
Copy link
Copy Markdown
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.

Comment thread clap_complete/src/dynamic/completer.rs Outdated
@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
Comment thread clap_complete/src/dynamic/completer.rs
@epage epage enabled auto-merge August 8, 2024 15:52
@epage epage merged commit 2690e1b into clap-rs:master Aug 8, 2024
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