-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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): Custom parser for arg values #3732
Merged
Merged
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
Unfortunately, we can't track using a `ValueParser` inside of `Command` because its `PartialEq`. We'll have to wait until a breaking change to relax that. Compatibility: - We now assert on using the wrong method to look up defaults. This shouldn't be a breaking change as it'll assert when getting a real value. - `values_of`, et al delay panicing on the wrong lookup until the iterator is being processed.
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
Still unsure what should be the default but this at least makes it easier for people to choose.
In theory, directly implemented types should have higher precedence than inferring from another trait.
This will let us replace `Arg::possible_values` completely by letting completions check these.
This identified a problem with the blanket implementations for `TypedValueParser`: it only worked on function pointer types and not unnamed function tupes. The user has to explicitly decay the function type to a function pointer to opt-in, so I changed the blanket impl. The loss of the `OsStr` impl shouldn't be too bad.
This gives a little more implementation flexibility
epage
force-pushed
the
domain
branch
2 times, most recently
from
May 17, 2022 22:05
fddfacd
to
7212661
Compare
This is needed for `Bound::cloned` and fits within official MSRV policy (2 versions back) and unofficial (6 months, see clap-rs#3267)
This was referenced May 18, 2022
epage
added a commit
to epage/clap
that referenced
this pull request
May 24, 2022
The remove functions no longer return `Arc` but the core type, at the cost of requiring `Clone`. I originally held off on this in clap-rs#3732 in the hope of gracefully transition the derive and requiring `Clone` would have been a breaking change but when it came to clap-rs#3734, I didn't find a way to make it work without a breaking change, so I made it opt-in. This means I can force the `Clone` requirement now. I added the requirement for `Clone` everywhere else in the hopes that in the future, we can drop the `Arc` without a breaking change.
This was referenced May 24, 2022
epage
added a commit
to epage/clap
that referenced
this pull request
May 25, 2022
Clap has focused on reporting development errors through assertions rather than mixing user errors with development errors. Sometimes, developers need to handle things more flexibly so included in clap-rs#3732 was the reporting of value accessor failures as internal errors with a distinct type. I've been going back and forth on whether the extra error pessimises the usability in the common case vs dealing with the proliferation of different function combinations. In working on deprecating the `value_of` functions, I decided that it was going to be worth duplicating so long as we can keep the documentation focused.
This was referenced May 25, 2022
This was referenced Jun 2, 2022
This was referenced Jun 9, 2022
Closed
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.
Currently clap is focused on raw values and hard codes support for converting to
String
s. Users could then set a validator on that but if they were parsing to validate, the result was thrown away.This moves clap to an API that let's users specify the data type / parser and we'll store the parsed value and return it to the user.
This is best shown with an example:
Arg::value_parser
:String
allow_invalid_utf8(true)
is calledvalue_parser
accepts anyInto<ValueParser>
, which includesTypedValueParser
including ones provided by clapvalue_parser!(T)
which will auto-select a parser from a type using deref specialization, inspired by feat: Auto-detectFromStr
,TryFrom<&OsStr>
, etc., in clap_derive #2298clap_derive
for a defaultvalue_parser
when none is specified.ArgMatches
:get_one::<T>
get_many::<T>
remove_one::<T>
remove_many::<T>
get_raw
for when generically walking over all of the data. This helped a lot with existing code (likevalidator_os
), so I went ahead and did it. We might be able to get rid of this in the future but it would require callingget_one
orget_many
for every possible data type.Overall, this helped more towards #3476 than I expected by allowing users to plugin their own logic. As an example, we were able to replace the following with
value_parser
:Arg::possible_values
forbid_empty_value
allow_invalid_utf8
validator
andvalidator_os
while all implemented in a way the compiler can more easily discard the parts of the API that are unused, helping us towards #1365
We also gained support for
ArgEnum
sValueHint::AnyPath
when using aPathBuf
parserThe builder API isn't as ergonomic as it was before but it is offering the user some more flexibility which is the main selling point of it over the builder API. For example,
ArgMatches::get_one
returns anOption
for when not present, rather than an error to help with #2505. Rather than panicing when theget_one
call is used with the wrong data type or the argument isn't defined, we now return an internal-facing enum to help with #3621.ArgMatches::remove_*
had to provide data inArc
s becauseArgMatches
implingClone
and didn't want to force the data to beClone
Arc
API I saw wastry_unwrap
but that leaves us with anArc
still if the ref count is more than 1.One limitation of this API is it doesn't allow using the full power of
ArgGroup
. Currently, clap allows you accessing all of the values for a group. The new implementation limits that to groups where all present values are of the same type. For more background on why this was originally added to clap, see #283. We might want to re-evaluate this.I had originally planned for the roll out of this to be staged and under
unstable-*
feature flags. I decided against this because of how close I feelclap4
is; we can always just change it then and this will get us feedback faster.Related changes
We now set the
source
andoccurrences
for external subcommands.A lot of refactors were made to the parsing logic to ensure we could pass all the
Arg
state we wanted down toMatchedArg
, even if there is an occurrence without a value. Unfortunately, we do not carry this state around for when there are no occurrences.Provide
ArgMatches::remove_subcommand
to allow for usingremove_one
/remove_many
on subcommands, especially forclap_derive
.Deferred
Appearing in the order to be completed
Adjusting the derive API to this is proving to be difficult because of the current traits take
&ArgMatches
and becauseFromArgMatches::update_from_arg_matches
derive for subcommands assumes thatArgMatches
remains untouched. See #3734. Implementing the derive will automatically addressWe have not yet deprecated the older builder methods as we probably want derive support implemented first so derive users do not get confused. See #3734.
We can't add a
ValueParser
to external subcommands because of thePartialEq
requirement and we can't relax that until a breaking release. See #3733._one
and_many
functions do not error if the wrong one is used. We might want to consider that. See #3735.Clarifying that
Arg::default_value
is for raw valuesProviding an
Arg::default_value_t
#2683 brought up the idea of defaulting
value_name
based on theValueParser
. This is still possible for us to look into, just not right now.Currently, all values must be of the same type. This assumption has kept things simpler for now. I'm assuming this is ok because the main situation for having position-specific types is when using a separator and the user can provide their own implementation.
Compatibility
MSRV is now 1.56.0. This is needed for
Bound::cloned
and fits within official MSRV policy(2 versions back) and unofficial (6 months, see #3267)
Because in clap 3.0.0, we tracked what is an
OsString
vsString
, we were able to graft this into the existing API except for external subcommands.We now assert on using the wrong method to look up defaults (
value_of
,value_of_os
). This shouldn't be a breaking change as it'll assert when getting a real value.Footer
Fixes #2683
Fixes #2505
Fixes #3621