From f1bf9fc250b0b75163da0fcdf5709f22d2c551ea Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 23 Oct 2021 15:52:48 -0500 Subject: [PATCH 1/5] refactor(error): Delay formatting until the end This gives us room to add extra context later. --- src/build/app/mod.rs | 4 +-- src/output/fmt.rs | 2 +- src/parse/errors.rs | 68 +++++++++++++++++++++++++++++++------------- src/util/color.rs | 2 +- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 8f1e48d8bbc..852469c40f1 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -2032,7 +2032,7 @@ impl<'help> App<'help> { .unwrap_or_else(|e| { // Otherwise, write to stderr and exit if e.use_stderr() { - e.message.print().expect("Error writing Error to stderr"); + e.print().expect("Error writing Error to stderr"); if self.settings.is_set(AppSettings::WaitOnError) { wlnerr!("\nPress [ENTER] / [RETURN] to continue..."); @@ -2114,7 +2114,7 @@ impl<'help> App<'help> { self.try_get_matches_from_mut(itr).unwrap_or_else(|e| { // Otherwise, write to stderr and exit if e.use_stderr() { - e.message.print().expect("Error writing Error to stderr"); + e.print().expect("Error writing Error to stderr"); if self.settings.is_set(AppSettings::WaitOnError) { wlnerr!("\nPress [ENTER] / [RETURN] to continue..."); diff --git a/src/output/fmt.rs b/src/output/fmt.rs index 267feb18053..9c15b1340d6 100644 --- a/src/output/fmt.rs +++ b/src/output/fmt.rs @@ -18,7 +18,7 @@ fn is_a_tty(stderr: bool) -> bool { atty::is(stream) } -#[derive(Debug)] +#[derive(Clone, Debug)] pub(crate) struct Colorizer { use_stderr: bool, color_when: ColorChoice, diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 682e6cf3aba..7ae3ab476f0 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -1,5 +1,6 @@ // Std use std::{ + borrow::Cow, convert::From, error, fmt::{self, Debug, Display, Formatter}, @@ -430,7 +431,7 @@ pub enum ErrorKind { #[derive(Debug)] pub struct Error { /// Formatted error message, enhancing the cause message with extra information - pub(crate) message: Colorizer, + pub(crate) message: Message, /// The type of error pub kind: ErrorKind, /// Additional information depending on the error kind, like values and argument names. @@ -444,14 +445,14 @@ impl Display for Error { fn fmt(&self, f: &mut Formatter) -> fmt::Result { #[cfg(feature = "debug")] { - writeln!(f, "{}", self.message)?; + writeln!(f, "{}", self.message.formatted())?; writeln!(f)?; writeln!(f, "Backtrace:")?; writeln!(f, "{}", self.backtrace)?; } #[cfg(not(feature = "debug"))] { - Display::fmt(&self.message, f)?; + Display::fmt(&self.message.formatted(), f)?; } Ok(()) } @@ -505,12 +506,12 @@ impl Error { pub fn exit(&self) -> ! { if self.use_stderr() { // Swallow broken pipe errors - let _ = self.message.print(); + let _ = self.print(); safe_exit(USAGE_CODE); } // Swallow broken pipe errors - let _ = self.message.print(); + let _ = self.print(); safe_exit(SUCCESS_CODE) } @@ -531,12 +532,12 @@ impl Error { /// }; /// ``` pub fn print(&self) -> io::Result<()> { - self.message.print() + self.message.formatted().print() } - pub(crate) fn new(message: Colorizer, kind: ErrorKind) -> Self { + pub(crate) fn new(message: impl Into, kind: ErrorKind) -> Self { Self { - message, + message: message.into(), kind, info: vec![], source: None, @@ -852,7 +853,12 @@ impl Error { err: Box, ) -> Self { let mut err = Self::value_validation_with_color(arg, val, err, app.get_color()); - try_help(app, &mut err.message); + match &mut err.message { + Message::Raw(_) => { + unreachable!("`value_validation_with_color` only deals in formatted errors") + } + Message::Formatted(c) => try_help(app, c), + } err } @@ -1001,26 +1007,19 @@ impl Error { /// [`App::error`]: crate::App::error #[deprecated(since = "3.0.0", note = "Replaced with `App::error`")] pub fn with_description(description: String, kind: ErrorKind) -> Self { - let mut c = Colorizer::new(true, ColorChoice::Auto); - - start_error(&mut c, description); - Error::new(c, kind) + Error::new(description, kind) } } impl From for Error { fn from(e: io::Error) -> Self { - let mut c = Colorizer::new(true, ColorChoice::Auto); - start_error(&mut c, e.to_string()); - Error::new(c, ErrorKind::Io) + Error::new(e.to_string(), ErrorKind::Io) } } impl From for Error { fn from(e: fmt::Error) -> Self { - let mut c = Colorizer::new(true, ColorChoice::Auto); - start_error(&mut c, e.to_string()); - Error::new(c, ErrorKind::Format) + Error::new(e.to_string(), ErrorKind::Format) } } @@ -1031,6 +1030,37 @@ impl error::Error for Error { } } +#[derive(Clone, Debug)] +pub(crate) enum Message { + Raw(String), + Formatted(Colorizer), +} + +impl Message { + fn formatted(&self) -> Cow { + match self { + Message::Raw(s) => { + let mut c = Colorizer::new(true, ColorChoice::Auto); + start_error(&mut c, s); + Cow::Owned(c) + } + Message::Formatted(c) => Cow::Borrowed(c), + } + } +} + +impl From for Message { + fn from(inner: String) -> Self { + Self::Raw(inner) + } +} + +impl From for Message { + fn from(inner: Colorizer) -> Self { + Self::Formatted(inner) + } +} + #[cfg(feature = "debug")] #[derive(Debug)] struct Backtrace(backtrace::Backtrace); diff --git a/src/util/color.rs b/src/util/color.rs index 825b98760b9..41b5cb79df1 100644 --- a/src/util/color.rs +++ b/src/util/color.rs @@ -65,7 +65,7 @@ impl Default for ColorChoice { pub(crate) use termcolor::Color; #[cfg(not(feature = "color"))] -#[derive(Debug)] +#[derive(Copy, Clone, Debug)] pub(crate) enum Color { Green, Yellow, From 83b074ae920c870d5b290edfb0f8cd1a1578995d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 23 Oct 2021 16:03:22 -0500 Subject: [PATCH 2/5] refactor(error): Extract formatting of raw messages --- src/parse/errors.rs | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 7ae3ab476f0..478b4e9d938 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -561,13 +561,9 @@ impl Error { kind: ErrorKind, message: impl std::fmt::Display, ) -> Self { - let mut c = Colorizer::new(true, app.get_color()); - - start_error(&mut c, message.to_string()); - put_usage(&mut c, usage); - try_help(app, &mut c); - - Self::new(c, kind) + let mut err = Self::new(message.to_string(), kind); + err.message.format(app, usage); + err } pub(crate) fn argument_conflict( @@ -1037,6 +1033,22 @@ pub(crate) enum Message { } impl Message { + fn format(&mut self, app: &App, usage: String) { + match self { + Message::Raw(s) => { + let mut c = Colorizer::new(true, app.get_color()); + + let mut message = String::new(); + std::mem::swap(s, &mut message); + start_error(&mut c, message); + put_usage(&mut c, usage); + try_help(app, &mut c); + *self = Self::Formatted(c); + } + Message::Formatted(_) => {} + } + } + fn formatted(&self) -> Cow { match self { Message::Raw(s) => { From 3c4340c5837f599741e7456fd88f0eba9653bfa4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 23 Oct 2021 16:30:32 -0500 Subject: [PATCH 3/5] feat(error): Allow separate raise, format sites While `App::error` is what most people will need, `clap_derive` needs to handle when the site raising the error doesn't have access to the `App` and needs to defer that to later. --- src/build/app/mod.rs | 4 +--- src/parse/errors.rs | 29 ++++++++++++++++++----------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 852469c40f1..0093e623a65 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -1813,9 +1813,7 @@ impl<'help> App<'help> { /// let err = app.error(ErrorKind::InvalidValue, "Some failure case"); /// ``` pub fn error(&mut self, kind: ErrorKind, message: impl std::fmt::Display) -> Error { - self._build(); - let usage = self.render_usage(); - Error::user_error(self, usage, kind, message) + Error::raw(kind, message).format(self) } /// Prints the full help message to [`io::stdout()`] using a [`BufWriter`] using the same diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 478b4e9d938..7f25be451c0 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -491,6 +491,24 @@ impl Error { } } + /// Create an unformatted error + /// + /// Prefer [`App::error`] for generating errors. This is for you need to pass the error up to + /// a place that has access to the `App` at which point you can call [`Error::format] + /// + /// [`App::error`]: crate::App::error + pub fn raw(kind: ErrorKind, message: impl std::fmt::Display) -> Self { + Self::new(message.to_string(), kind) + } + + /// Format the existing message with the App's context + pub fn format(mut self, app: &mut App) -> Self { + app._build(); + let usage = app.render_usage(); + self.message.format(app, usage); + self + } + /// Should the message be written to `stdout` or not #[inline] pub fn use_stderr(&self) -> bool { @@ -555,17 +573,6 @@ impl Error { self } - pub(crate) fn user_error( - app: &App, - usage: String, - kind: ErrorKind, - message: impl std::fmt::Display, - ) -> Self { - let mut err = Self::new(message.to_string(), kind); - err.message.format(app, usage); - err - } - pub(crate) fn argument_conflict( app: &App, arg: &Arg, From f8bca3a84bd54a2cb43c573868c397bec1e1c043 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 25 Oct 2021 09:24:28 -0500 Subject: [PATCH 4/5] fix(error): Never show unrequested color If the user prints a raw error, it may include color even if the user turned it off at runtime. Now we'll be more conservative and never show color for raw errors. --- src/parse/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 7f25be451c0..6a90567862d 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -1059,7 +1059,7 @@ impl Message { fn formatted(&self) -> Cow { match self { Message::Raw(s) => { - let mut c = Colorizer::new(true, ColorChoice::Auto); + let mut c = Colorizer::new(true, ColorChoice::Never); start_error(&mut c, s); Cow::Owned(c) } From 53e10b41e34a9b494b26d3d3b6cf2765b954257c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 23 Oct 2021 16:30:32 -0500 Subject: [PATCH 5/5] fix(derive)!: Error, don't panic! The derive is generating `Error::raw` (from scratch or by converting existing erors) and then the inherent `Parser` methods format them. Fixes #2255 --- clap_derive/src/derives/args.rs | 72 ++++++++++++++++----------- clap_derive/src/derives/subcommand.rs | 29 ++++++----- clap_derive/src/dummies.rs | 4 +- src/derive.rs | 53 +++++++++++++++----- 4 files changed, 101 insertions(+), 57 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 4bb24710b6c..6f7a0e6e699 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -118,13 +118,14 @@ pub fn gen_from_arg_matches_for_struct( )] #[deny(clippy::correctness)] impl clap::FromArgMatches for #struct_name { - fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Option { + fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result { let v = #struct_name #constructor; - Some(v) + ::std::result::Result::Ok(v) } - fn update_from_arg_matches(&mut self, __clap_arg_matches: &clap::ArgMatches) { + fn update_from_arg_matches(&mut self, __clap_arg_matches: &clap::ArgMatches) -> Result<(), clap::Error> { #updater + ::std::result::Result::Ok(()) } } } @@ -379,20 +380,30 @@ pub fn gen_constructor(fields: &Punctuated, parent_attribute: &Att (Ty::Option, Some(sub_type)) => sub_type, _ => &field.ty, }; - let unwrapper = match **ty { - Ty::Option => quote!(), - _ => quote_spanned!( ty.span()=> .expect("app should verify subcommand is required") ), - }; - quote_spanned! { kind.span()=> - #field_name: { - <#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches) - #unwrapper - } + match **ty { + Ty::Option => { + quote_spanned! { kind.span()=> + #field_name: { + if #arg_matches.subcommand_name().map(<#subcmd_type as clap::Subcommand>::has_subcommand).unwrap_or(false) { + Some(<#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches)?) + } else { + None + } + } + } + }, + _ => { + quote_spanned! { kind.span()=> + #field_name: { + <#subcmd_type as clap::FromArgMatches>::from_arg_matches(#arg_matches)? + } + } + }, } } Kind::Flatten => quote_spanned! { kind.span()=> - #field_name: clap::FromArgMatches::from_arg_matches(#arg_matches).unwrap() + #field_name: clap::FromArgMatches::from_arg_matches(#arg_matches)? }, Kind::Skip(val) => match val { @@ -448,7 +459,7 @@ pub fn gen_updater( }; let updater = quote_spanned! { ty.span()=> - <#subcmd_type as clap::FromArgMatches>::update_from_arg_matches(#field_name, #arg_matches); + <#subcmd_type as clap::FromArgMatches>::update_from_arg_matches(#field_name, #arg_matches)?; }; let updater = match **ty { @@ -456,9 +467,9 @@ pub fn gen_updater( if let Some(#field_name) = #field_name.as_mut() { #updater } else { - *#field_name = <#subcmd_type as clap::FromArgMatches>::from_arg_matches( + *#field_name = Some(<#subcmd_type as clap::FromArgMatches>::from_arg_matches( #arg_matches - ) + )?); } }, _ => quote_spanned! { kind.span()=> @@ -476,7 +487,7 @@ pub fn gen_updater( Kind::Flatten => quote_spanned! { kind.span()=> { #access - clap::FromArgMatches::update_from_arg_matches(#field_name, #arg_matches); + clap::FromArgMatches::update_from_arg_matches(#field_name, #arg_matches)?; } }, @@ -504,26 +515,27 @@ fn gen_parsers( let func = &parser.func; let span = parser.kind.span(); let convert_type = inner_type(**ty, &field.ty); + let name = attrs.cased_name(); let (value_of, values_of, mut parse) = match *parser.kind { FromStr => ( quote_spanned!(span=> value_of), quote_spanned!(span=> values_of), - func.clone(), + quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(#func(s))), ), TryFromStr => ( quote_spanned!(span=> value_of), quote_spanned!(span=> values_of), - quote_spanned!(func.span()=> |s| #func(s).unwrap()), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), ), FromOsStr => ( quote_spanned!(span=> value_of_os), quote_spanned!(span=> values_of_os), - func.clone(), + quote_spanned!(func.span()=> |s| ::std::result::Result::Ok::<_, clap::Error>(#func(s))), ), TryFromOsStr => ( quote_spanned!(span=> value_of_os), quote_spanned!(span=> values_of_os), - quote_spanned!(func.span()=> |s| #func(s).unwrap()), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), ), FromOccurrences => ( quote_spanned!(span=> occurrences_of), @@ -536,13 +548,12 @@ fn gen_parsers( let ci = attrs.case_insensitive(); parse = quote_spanned! { convert_type.span()=> - |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).expect("app should verify the choice was valid") + |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err))) } } let flag = *attrs.parser().kind == ParserKind::FromFlag; let occurrences = *attrs.parser().kind == ParserKind::FromOccurrences; - let name = attrs.cased_name(); // Give this identifier the same hygiene // as the `arg_matches` parameter definition. This // allows us to refer to `arg_matches` within a `quote_spanned` block @@ -565,12 +576,13 @@ fn gen_parsers( quote_spanned! { ty.span()=> #arg_matches.#value_of(#name) .map(#parse) + .transpose()? } } Ty::OptionOption => quote_spanned! { ty.span()=> if #arg_matches.is_present(#name) { - Some(#arg_matches.#value_of(#name).map(#parse)) + Some(#arg_matches.#value_of(#name).map(#parse).transpose()?) } else { None } @@ -579,8 +591,9 @@ fn gen_parsers( Ty::OptionVec => quote_spanned! { ty.span()=> if #arg_matches.is_present(#name) { Some(#arg_matches.#values_of(#name) - .map(|v| v.map::<#convert_type, _>(#parse).collect()) - .unwrap_or_else(Vec::new)) + .map(|v| v.map::, _>(#parse).collect::, clap::Error>>()) + .transpose()? + .unwrap_or_else(Vec::new)) } else { None } @@ -589,7 +602,8 @@ fn gen_parsers( Ty::Vec => { quote_spanned! { ty.span()=> #arg_matches.#values_of(#name) - .map(|v| v.map::<#convert_type, _>(#parse).collect()) + .map(|v| v.map::, _>(#parse).collect::, clap::Error>>()) + .transpose()? .unwrap_or_else(Vec::new) } } @@ -605,8 +619,8 @@ fn gen_parsers( Ty::Other => { quote_spanned! { ty.span()=> #arg_matches.#value_of(#name) - .map(#parse) - .expect("app should verify arg is required") + .ok_or_else(|| clap::Error::raw(clap::ErrorKind::MissingRequiredArgument, format!("The following required argument was not provided: {}", #name))) + .and_then(#parse)? } } }; diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs index cea16f2fe43..6edb0c13ec4 100644 --- a/clap_derive/src/derives/subcommand.rs +++ b/clap_derive/src/derives/subcommand.rs @@ -448,14 +448,14 @@ fn gen_from_arg_matches( Unit => quote!(), Unnamed(ref fields) if fields.unnamed.len() == 1 => { let ty = &fields.unnamed[0]; - quote!( ( <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches).unwrap() ) ) + quote!( ( <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches)? ) ) } Unnamed(..) => abort_call_site!("{}: tuple enums are not supported", variant.ident), }; quote! { if #sub_name == #subcommand_name_var { - return Some(#name :: #variant_name #constructor_block) + return ::std::result::Result::Ok(#name :: #variant_name #constructor_block) } } }); @@ -466,8 +466,8 @@ fn gen_from_arg_matches( let ty = &fields.unnamed[0]; quote! { if <#ty as clap::Subcommand>::has_subcommand(__clap_name) { - let __clap_res = <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches).unwrap(); - return Some(#name :: #variant_name (__clap_res)); + let __clap_res = <#ty as clap::FromArgMatches>::from_arg_matches(__clap_arg_matches)?; + return ::std::result::Result::Ok(#name :: #variant_name (__clap_res)); } } } @@ -480,7 +480,7 @@ fn gen_from_arg_matches( let wildcard = match ext_subcmd { Some((span, var_name, str_ty, values_of)) => quote_spanned! { span=> - ::std::option::Option::Some(#name::#var_name( + ::std::result::Result::Ok(#name::#var_name( ::std::iter::once(#str_ty::from(#subcommand_name_var)) .chain( #sub_arg_matches_var.#values_of("").into_iter().flatten().map(#str_ty::from) @@ -489,11 +489,13 @@ fn gen_from_arg_matches( )) }, - None => quote!(None), + None => quote! { + ::std::result::Result::Err(clap::Error::raw(clap::ErrorKind::UnrecognizedSubcommand, format!("The subcommand '{}' wasn't recognized", #subcommand_name_var))) + }, }; quote! { - fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Option { + fn from_arg_matches(__clap_arg_matches: &clap::ArgMatches) -> Result { if let Some((#subcommand_name_var, #sub_arg_matches_var)) = __clap_arg_matches.subcommand() { { let __clap_arg_matches = #sub_arg_matches_var; @@ -504,7 +506,7 @@ fn gen_from_arg_matches( #wildcard } else { - None + ::std::result::Result::Err(clap::Error::raw(clap::ErrorKind::MissingSubcommand, "A subcommand is required but one was not provided.")) } } } @@ -568,7 +570,7 @@ fn gen_update_from_arg_matches( quote!(clap::FromArgMatches::update_from_arg_matches( __clap_arg, __clap_sub_arg_matches - )), + )?), ) } else { abort_call_site!("{}: tuple enums are not supported", variant.ident) @@ -592,8 +594,8 @@ fn gen_update_from_arg_matches( quote! { if <#ty as clap::Subcommand>::has_subcommand(__clap_name) { if let #name :: #variant_name (child) = s { - <#ty as clap::FromArgMatches>::update_from_arg_matches(child, __clap_arg_matches); - return; + <#ty as clap::FromArgMatches>::update_from_arg_matches(child, __clap_arg_matches)?; + return ::std::result::Result::Ok(()); } } } @@ -609,16 +611,17 @@ fn gen_update_from_arg_matches( fn update_from_arg_matches<'b>( &mut self, __clap_arg_matches: &clap::ArgMatches, - ) { + ) -> Result<(), clap::Error> { if let Some((__clap_name, __clap_sub_arg_matches)) = __clap_arg_matches.subcommand() { match self { #( #subcommands ),* s => { #( #child_subcommands )* - *s = ::from_arg_matches(__clap_arg_matches).unwrap(); + *s = ::from_arg_matches(__clap_arg_matches)?; } } } + ::std::result::Result::Ok(()) } } } diff --git a/clap_derive/src/dummies.rs b/clap_derive/src/dummies.rs index 40250eaa8ae..2314a05cf62 100644 --- a/clap_derive/src/dummies.rs +++ b/clap_derive/src/dummies.rs @@ -32,10 +32,10 @@ pub fn into_app(name: &Ident) { pub fn from_arg_matches(name: &Ident) { append_dummy(quote! { impl clap::FromArgMatches for #name { - fn from_arg_matches(_m: &clap::ArgMatches) -> Option { + fn from_arg_matches(_m: &clap::ArgMatches) -> Result { unimplemented!() } - fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) { + fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) -> Result<(), clap::Error>{ unimplemented!() } } diff --git a/src/derive.rs b/src/derive.rs index e73ff4f06e6..c0477a3aa90 100644 --- a/src/derive.rs +++ b/src/derive.rs @@ -74,14 +74,22 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { /// Parse from `std::env::args_os()`, exit on error fn parse() -> Self { let matches = ::into_app().get_matches(); - ::from_arg_matches(&matches).expect("IntoApp validated everything") + let res = + ::from_arg_matches(&matches).map_err(format_error::); + match res { + Ok(s) => s, + Err(e) => { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches` + e.exit() + } + } } /// Parse from `std::env::args_os()`, return Err on error. fn try_parse() -> Result { let matches = ::into_app().try_get_matches()?; - Ok(::from_arg_matches(&matches) - .expect("IntoApp validated everything")) + ::from_arg_matches(&matches).map_err(format_error::) } /// Parse from iterator, exit on error @@ -92,7 +100,16 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app().get_matches_from(itr); - ::from_arg_matches(&matches).expect("IntoApp validated everything") + let res = + ::from_arg_matches(&matches).map_err(format_error::); + match res { + Ok(s) => s, + Err(e) => { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches_from` + e.exit() + } + } } /// Parse from iterator, return Err on error. @@ -103,8 +120,7 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app().try_get_matches_from(itr)?; - Ok(::from_arg_matches(&matches) - .expect("IntoApp validated everything")) + ::from_arg_matches(&matches).map_err(format_error::) } /// Update from iterator, exit on error @@ -116,7 +132,13 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { { // TODO find a way to get partial matches let matches = ::into_app_for_update().get_matches_from(itr); - ::update_from_arg_matches(self, &matches); + let res = ::update_from_arg_matches(self, &matches) + .map_err(format_error::); + if let Err(e) = res { + // Since this is more of a development-time error, we aren't doing as fancy of a quit + // as `get_matches_from` + e.exit() + } } /// Update from iterator, return Err on error. @@ -127,8 +149,8 @@ pub trait Parser: FromArgMatches + IntoApp + Sized { T: Into + Clone, { let matches = ::into_app_for_update().try_get_matches_from(itr)?; - ::update_from_arg_matches(self, &matches); - Ok(()) + ::update_from_arg_matches(self, &matches) + .map_err(format_error::) } } @@ -178,10 +200,10 @@ pub trait FromArgMatches: Sized { /// } /// } /// ``` - fn from_arg_matches(matches: &ArgMatches) -> Option; + fn from_arg_matches(matches: &ArgMatches) -> Result; /// Assign values from `ArgMatches` to `self`. - fn update_from_arg_matches(&mut self, matches: &ArgMatches); + fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error>; } /// Parse arguments into a user-defined container. @@ -347,10 +369,10 @@ impl IntoApp for Box { } impl FromArgMatches for Box { - fn from_arg_matches(matches: &ArgMatches) -> Option { + fn from_arg_matches(matches: &ArgMatches) -> Result { ::from_arg_matches(matches).map(Box::new) } - fn update_from_arg_matches(&mut self, matches: &ArgMatches) { + fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), Error> { ::update_from_arg_matches(self, matches) } } @@ -375,3 +397,8 @@ impl Subcommand for Box { ::has_subcommand(name) } } + +fn format_error(err: crate::Error) -> crate::Error { + let mut app = I::into_app(); + err.format(&mut app) +}