-
-
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
clap_derive: support fixed arrays as shortcut for number_of_values = N #3661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
I know this is still in Draft but called out some of the things that need to be resolved before this is ready to go
03246d2
to
e5344d3
Compare
@epage ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the errors and this is good to go!
clap_derive/src/derives/args.rs
Outdated
.map(|v| v.map::<::std::result::Result<#convert_type, clap::Error>, _>(#parse).collect::<::std::result::Result<Vec<_>, clap::Error>>()) | ||
.transpose()? | ||
.unwrap_or_else(Vec::new) | ||
).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please report an error instead of doing an unwrap
.
Before clap 3.0 was released, we did an audit where we removed unwrap
s in FromArgMatches
that assumed how the arg was setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorKind::Internal
added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to add an Internal
over using something like WrongNumberOfValues
? All of the other derive errors like this return the public error kind that the validator would in that circumstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error only emits when clap itself is wrong. It's unreachable. clap ensures that the returned vec has the same length as the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the error from a mismatch between the derived Args
impl and the derived FromArgMatches
impl? Those are what we were changing from unwrap
to public errors.
Reasons for public errors in these cases
- There have been unexpected cases where the user passed valid attributes and missed a corner case wasn't handled in the derive, causing it to panic instead of error. iirc this was a bug in clap and ideally would have been caught when the developer was testing the application but the combination of application states to test is large and its better to provide a nice error message to the end-user than to crash or report an internal error message. We made sure that with
--features clap/debug
, we will report a back trace so users can root cause what is happening - We've made the trait behaviors public, unlike with structopt, and want to allow the user to use the two halves of the derive as they see fit and we should gracefully handle this rather than crash or report internal errors
clap_derive/src/derives/args.rs
Outdated
.map(|v| v.map::<::std::result::Result<#convert_type, clap::Error>, _>(#parse).collect::<::std::result::Result<Vec<_>, clap::Error>>()) | ||
.transpose()? | ||
.unwrap_or_else(Vec::new) | ||
).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about unwrap
} else if cfg!(feature = "unstable-array") && is_array_ty(ty) { | ||
t(Array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot another use case: how do people opt-out?
I don't think we are really documenting it at the moment but the way to opt-out of the other built-in types is by fully qualifying the path but I'm unsure if there is a way to do this.
There might be times when a user wants to parse a single argument into one of these types. We already provide an example for a tuple (examples/typed-derive.rs
) and I recently stepped someone through how to do it with Vec
, its not a stretch to assume people will need to do that for this case as well.
Maybe the opt-out is std::option::Option<[...]>
and they manually mark it as required = true
? Whatever we decide, we should at least have a test for it.
This is split out of clap-rs#3661 as several changes I'm working on need it.
ec78867
to
f20e5f1
Compare
Add `ErrorKind::Internal` for erroring
Closing out due to the conflicts and lack of activity. |
fixes #1682