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(builder): Allow injecting known unknowns #5075

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Conversation

epage
Copy link
Member

@epage epage commented Aug 16, 2023

Fixes #4706

Copy link

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty much fits Cargo's need!

Can we do this on Cargo side, without making it a built-in in clap? So that clap has one less thing to maintain, and Cargo can customize the message more freely.

(I guess the answer is yes with TypedValueParser)

@epage
Copy link
Member Author

epage commented Aug 16, 2023

I think this makes sense to provide generally. Also, this uses an internal error constructor that cargo would need to reproduce. I've not made it public because no work has been put into defining what it should look like but doing the manual work isn't exactly pretty.

Are there any thoughts of how you think we'd want to customize it for cargo?

@weihanglo
Copy link

Like the proposed solution in rust-lang/cargo#12009 has a nicer context. The approach in this PR is not bad though. Better than nothing 😆.

@epage
Copy link
Member Author

epage commented Aug 16, 2023

To be clear, I believe this works for most cases as they follow the pattern of there being a specific alternative. This only breaks down when there isn't an alternative, like the behavior being the default.

Personally, I would consider supporting both flags, with the default being hidden and produces a warning. However, I think we can improve this PR for more situations. Clap's semi-structured errors supports a specific arg suggestion and a free form suggestion. We can support both here. However, we don't currently support both "note"s, only "tip"s (ie any solution to have "note" would be a bit hacky).

While there might be more specialized cases we should leave to callers, I think this still provides something generally useful and taking input on where its not useful is important to the process.

clap_builder/src/builder/value_parser.rs Outdated Show resolved Hide resolved
clap_builder/src/builder/value_parser.rs Outdated Show resolved Hide resolved
@epage
Copy link
Member Author

epage commented Aug 17, 2023

@weihanglo the API has been updated to allow a bit more flexibility. I'm assuming this will now handle most of the common cases.

Copy link

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. You just maximized the flexibility 👍🏾.

@epage epage merged commit b87ca2f into clap-rs:master Aug 17, 2023
22 checks passed
@epage epage deleted the err branch August 17, 2023 13:58
Carreau added a commit to Carreau/clap that referenced this pull request Aug 28, 2023
As pointed out in clap-rs#4853, it might be useful to mark arguments to not be
considered by the suggestions feature.

In particular with the introduction of UnknownArgumentValueParser in clap-rs#5075,
one might want explicitly handle some common confusion (say handle
`--silent` and print `tip: did you mean --quiet`), but not have those in
the suggestion engine. I'm guessing one might want to have an `arg` be visible
but not in suggestion for deprecated flags as well.

I'm trying to do so by adding a `.didyoumena(false)`, that will mark the
argument as to not be considered by suggestion.

There is also some mention of completion of hidden command in clap-rs#4853,
that might be relevant as well.

--

I'm not too familiar with clap internals, and fairly new to rust,
but in particular :

 - I've tried to implement that only for args - I guess we will need to
   extend to subcommands if this goes further
 - I'm not happy with the setting negations nor the naming
   `didyoumean`/`ExcludeDidYouMean`
 - I'm not super happy about the iteration over MKeyMap keys and items.
Carreau added a commit to Carreau/clap that referenced this pull request Aug 28, 2023
As pointed out in clap-rs#4853, it might be useful to mark arguments to not be
considered by the suggestions feature.

In particular with the introduction of UnknownArgumentValueParser in clap-rs#5075,
one might want explicitly handle some common confusion (say handle
`--silent` and print `tip: did you mean --quiet`), but not have those in
the suggestion engine. I'm guessing one might want to have an `arg` be visible
but not in suggestion for deprecated flags as well.

I'm trying to do so by adding a `.didyoumena(false)`, that will mark the
argument as to not be considered by suggestion.

There is also some mention of completion of hidden command in clap-rs#4853,
that might be relevant as well.

--

I'm not too familiar with clap internals, and fairly new to rust,
but in particular :

 - I've tried to implement that only for args - I guess we will need to
   extend to subcommands if this goes further
 - I'm not happy with the setting negations nor the naming
   `didyoumean`/`ExcludeDidYouMean`
 - I'm not super happy about the iteration over MKeyMap keys and items.
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.

Support custom suggested fix messages for specific flags
2 participants