feat: Auto-detect FromStr, TryFrom<&OsStr>, etc., in clap_derive
#2298
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently the way `clap_derive` uses a `from_str` function is to call it once as a validator, discarding the parsed value, and then to call it again to recompute the parsed value. This is unfortunate in cases where `from_str` is expensive or has side effects. This PR changes `clap_derive` to not register `from_str` as a validator so that it doesn't do the first of these two calls. Then, instead of doing `unwrap()` on the other call, it handles the error. This eliminates the redundancy, and also avoids the small performance hit mentioned in [the documentation about validators]. [the documentation about validators]: https://docs.rs/clap-v3/3.0.0-beta.1/clap_v3/struct.Arg.html#method.validator This PR doesn't yet use colorized messages for errors generated during parsing because the `ColorWhen` setting isn't currently available. That's fixable with some refactoring, but I'm interested in getting feedback on the overall approach here first.
sunfishcode
force-pushed
the
auto
branch
2 times, most recently
from
291a85b
to
1805a97
Compare
Clap has several different options for parsing strings into values: `try_from_str`, `try_from_os_str`, `from_str`, `from_os_str`, each corresponding to a different way of parsing strings. This patch adds a new option: `auto`, which auto-detects which parsing traits a type supports, out of `clap::ArgEnum`, `TryFrom<&OsStr>`, `FromStr`, `TryFrom<&str>`, `From<&OsStr>`, and `From<&str>`, and selects the best one to use automatically. This way, users can use any type which implements any of these traits, and clap_derive automatically figures out what to do. It's implemented via [autoref specialization], a limited form of specialization which only works in macro contexts, but that turns out to be enough for clap_derive. This changes the default to `auto`. The previous default `try_from_str` uses the `FromStr` trait. `auto` auto-detects `FromStr`, so this is a mostly backwards-compatible change. [autoref specialization]: http://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html
pksunkara
requested changes
Jan 24, 2021
|
I took a quick look at separating out this PR, but it would need to be reorganized to emit all the inference code twice, once for validation and once for parsing, so it's more involved than a simple cherry-pick. |
|
#2206 will probably be the last thing to be done for 3.0. Would really appreciate if you can split stuff. |
5 tasks
|
I did look at splitting this out. However, I don't understand the purpose of organizing the code this way, so I'm unable to work on this. |
epage
added a commit
to epage/clap
that referenced
this pull request
May 17, 2022
To set the type, we offer - `ValueParser::<type>` short cuts for natively supported types - `TypedValueParser` for fn pointers and custom implementations - `value_parser!(T)` for specialized lookup of an implementation (inspired by clap-rs#2298) The main motivation for `value_parser!` is to help with `clap_derive`s implementation but it can also be convinient for end-users. When reading, this replaces nearly all of our current `ArgMatches` getters with: - `get_one`: like `value_of_t` - `get_many`: like `values_of_t` It also adds a `get_raw` that allows accessing the `OsStr`s without panicing. The naming is to invoke the idea of a more general container which I want to move this to. The return type is a bit complicated so that - Users choose whether to panic on invalid types so they can do their own querying, like `get_raw` - Users can choose how to handle not present easily (clap-rs#2505) We had to defer setting the `value_parser` on external subcommands, for consistency sake, because `Command` requires `PartialEq` and `ValueParser` does not impl that trait. It'll have to wait until a breaking change. Fixes clap-rs#2505
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background: I just learned about autoref specialization, and tried it out in clap_derive for autodetecting trait implementations. It appears to work really well for my purposes at least, so I thought I'd post the patch here to see what people think.
I based this PR on the patch in #2206 because my project which needs this needs both, but in principle the two changes could be separated.
Commit message
Clap has several different options for parsing strings into values:
try_from_str,try_from_os_str,from_str,from_os_str, eachcorresponding to a different way of parsing strings.
This patch adds a new option:
auto, which auto-detects which parsingtraits a type supports, out of
clap::ArgEnum,TryFrom<&OsStr>,FromStr,TryFrom<&str>,From<&OsStr>, andFrom<&str>, and selectsthe best one to use automatically. This way, users can use any type which
implements any of these traits, and clap_derive automatically figures out
what to do.
It's implemented via autoref specialization, a limited form of
specialization which only works in macro contexts, but that turns out to
be enough for clap_derive.
This changes the default to
auto. The previous defaulttry_from_struses the
FromStrtrait.autoauto-detectsFromStr, so this is amostly backwards-compatible change.