Skip to content

Commit

Permalink
refactor(derive): Simplify 'error' handling
Browse files Browse the repository at this point in the history
Our goal is to not panic inside of the macro (e.g. clap-rs#2255).  Currently,
we `.unwrap()` everywhere except when turning an `ArgMatches` into an
`enum`.  To handle `flatten`, we walk through each `flatten`ed enum to
see if it can be instantiated.  We don't want to mix this up with any of
the other eror cases (including further nested versions of this).

If we went straight to `Result<T>`, we'd have to differentiate this case
through the `ErrorKind` and `map_err` it in all other cases to prevent
it from bubbling up and confusing us.

Alternatively, we could do a more complicated type `Result<Option<T>>`
where the `Option` exists purely for this case and we can use type
checking to make sure we properly turn the `None`s into errors when
needed.

Or we can bypass all of this and just ask the `flatten`ed` subcommand if
it supports the current command.
  • Loading branch information
epage committed Oct 15, 2021
1 parent bfd6f8d commit 7b56184
Showing 1 changed file with 3 additions and 4 deletions.
7 changes: 3 additions & 4 deletions clap_derive/src/derives/subcommand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,8 @@ fn gen_from_arg_matches(
Unnamed(ref fields) if fields.unnamed.len() == 1 => {
let ty = &fields.unnamed[0];
quote! {
if let Some(res) = <#ty as clap::FromArgMatches>::from_arg_matches(arg_matches) {
if <#ty as clap::Subcommand>::has_subcommand(name) {
let res = <#ty as clap::FromArgMatches>::from_arg_matches(arg_matches).unwrap();
return Some(#name :: #variant_name (res));
}
}
Expand Down Expand Up @@ -600,9 +601,7 @@ fn gen_update_from_arg_matches(
#( #subcommands ),*
s => {
#( #child_subcommands )*
if let Some(sub) = <Self as clap::FromArgMatches>::from_arg_matches(arg_matches) {
*s = sub;
}
*s = <Self as clap::FromArgMatches>::from_arg_matches(arg_matches).unwrap();
}
}
}
Expand Down

0 comments on commit 7b56184

Please sign in to comment.