From 15ad5954b0d384405e3cccda47fd3fa9ed3af39f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Campinas?= Date: Tue, 27 Nov 2018 21:02:19 +0100 Subject: [PATCH 1/3] feat(Error): add a cause field to the Error struct The cause field is the actual error described in the message field, however without extra information like usage. Close #861 --- src/parse/errors.rs | 108 ++++++++++++++++++++++++++++++++++------- src/parse/parser.rs | 47 +++++++----------- src/parse/validator.rs | 9 +--- 3 files changed, 109 insertions(+), 55 deletions(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 45ce5708..59d6dda4 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -374,7 +374,9 @@ pub enum ErrorKind { /// Command Line Argument Parser Error #[derive(Debug)] pub struct Error { - /// Formatted error message + /// The cause of the error + pub cause: String, + /// Formatted error message, enhancing the cause message with extra information pub message: String, /// The type of error pub kind: ErrorKind, @@ -383,6 +385,15 @@ pub struct Error { } impl Error { + /// Returns the singular or plural form on the verb to be based on the argument's value. + fn singular_or_plural(n: usize) -> String { + if n > 1 { + String::from("were") + } else { + String::from("was") + } + } + /// Should the message be written to `stdout` or not pub fn use_stderr(&self) -> bool { match self.kind { @@ -454,21 +465,26 @@ impl Error { use_stderr: true, when: color, }); + let conflicting_arg = match other { + Some(name) => { + let n = name.into(); + v.push(n.clone()); + c.warning(format!("'{}'", n)) + } + None => c.none("one or more of the other specified arguments".to_owned()), + }; Error { + cause: format!( + "The argument '{}' cannot be used with {}", + arg, conflicting_arg + ), message: format!( "{} The argument '{}' cannot be used with {}\n\n\ {}\n\n\ For more information try {}", c.error("error:"), c.warning(&*arg.to_string()), - match other { - Some(name) => { - let n = name.into(); - v.push(n.clone()); - c.warning(format!("'{}'", n)) - } - None => c.none("one or more of the other specified arguments".to_owned()), - }, + conflicting_arg, usage, c.good("--help") ), @@ -487,6 +503,10 @@ impl Error { when: color, }); Error { + cause: format!( + "The argument '{}' requires a value but none was supplied", + arg + ), message: format!( "{} The argument '{}' requires a value but none was supplied\ \n\n\ @@ -529,6 +549,13 @@ impl Error { sorted.sort(); let valid_values = sorted.join(", "); Error { + cause: format!( + "'{}' isn't a valid value for '{}'\n\t\ + [possible values: {}]", + bad_val.as_ref(), + arg, + valid_values + ), message: format!( "{} '{}' isn't a valid value for '{}'\n\t\ [possible values: {}]\n\ @@ -568,6 +595,7 @@ impl Error { when: color, }); Error { + cause: format!("The subcommand '{}' wasn't recognized", s), message: format!( "{} The subcommand '{}' wasn't recognized\n\t\ Did you mean '{}'?\n\n\ @@ -601,6 +629,7 @@ impl Error { when: color, }); Error { + cause: format!("The subcommand '{}' wasn't recognized", s), message: format!( "{} The subcommand '{}' wasn't recognized\n\n\ {}\n\t\ @@ -628,6 +657,10 @@ impl Error { when: color, }); Error { + cause: format!( + "The following required arguments were not provided:{}", + required + ), message: format!( "{} The following required arguments were not provided:{}\n\n\ {}\n\n\ @@ -653,6 +686,7 @@ impl Error { when: color, }); Error { + cause: format!("'{}' requires a subcommand, but one was not provided", name), message: format!( "{} '{}' requires a subcommand, but one was not provided\n\n\ {}\n\n\ @@ -677,6 +711,7 @@ impl Error { when: color, }); Error { + cause: "Invalid UTF-8 was detected in one or more arguments".to_string(), message: format!( "{} Invalid UTF-8 was detected in one or more arguments\n\n\ {}\n\n\ @@ -702,6 +737,10 @@ impl Error { when: color, }); Error { + cause: format!( + "The value '{}' was provided to '{}', but it wasn't expecting any more values", + v, arg + ), message: format!( "{} The value '{}' was provided to '{}', but it wasn't expecting \ any more values\n\n\ @@ -733,9 +772,14 @@ impl Error { use_stderr: true, when: color, }); + let verb = Error::singular_or_plural(curr_vals); Error { + cause: format!( + "The argument '{}' requires at least {} values, but only {} {} provided", + arg, min_vals, curr_vals, verb + ), message: format!( - "{} The argument '{}' requires at least {} values, but only {} w{} \ + "{} The argument '{}' requires at least {} values, but only {} {} \ provided\n\n\ {}\n\n\ For more information try {}", @@ -743,7 +787,7 @@ impl Error { c.warning(arg.to_string()), c.warning(min_vals.to_string()), c.warning(curr_vals.to_string()), - if curr_vals > 1 { "ere" } else { "as" }, + verb, usage, c.good("--help") ), @@ -759,6 +803,15 @@ impl Error { when: color, }); Error { + cause: format!( + "Invalid value{}: {}", + if let Some(a) = arg { + format!(" for '{}'", a) + } else { + String::new() + }, + err + ), message: format!( "{} Invalid value{}: {}", c.error("error:"), @@ -781,25 +834,28 @@ impl Error { } #[doc(hidden)] - pub fn wrong_number_of_values( + pub fn wrong_number_of_values( arg: &Arg, num_vals: u64, curr_vals: usize, - suffix: S, usage: U, color: ColorWhen, ) -> Self where - S: Display, U: Display, { let c = Colorizer::new(&ColorizerOption { use_stderr: true, when: color, }); + let verb = Error::singular_or_plural(curr_vals); Error { + cause: format!( + "The argument '{}' requires {} values, but {} {} provided", + arg, num_vals, curr_vals, verb + ), message: format!( - "{} The argument '{}' requires {} values, but {} w{} \ + "{} The argument '{}' requires {} values, but {} {} provided\n\n\ {}\n\n\ For more information try {}", @@ -807,7 +863,7 @@ impl Error { c.warning(arg.to_string()), c.warning(num_vals.to_string()), c.warning(curr_vals.to_string()), - suffix, + verb, usage, c.good("--help") ), @@ -826,6 +882,10 @@ impl Error { when: color, }); Error { + cause: format!( + "The argument '{}' was provided more than once, but cannot be used multiple times", + arg + ), message: format!( "{} The argument '{}' was provided more than once, but cannot \ be used multiple times\n\n\ @@ -842,7 +902,12 @@ impl Error { } #[doc(hidden)] - pub fn unknown_argument(arg: A, did_you_mean: Option, usage: U, color: ColorWhen) -> Self + pub fn unknown_argument( + arg: A, + did_you_mean: Option, + usage: U, + color: ColorWhen, + ) -> Self where A: Into, U: Display, @@ -857,10 +922,14 @@ impl Error { let did_you_mean_message = match did_you_mean { Some(s) => format!("{}\n", s), - _ => "\n".to_owned() + _ => "\n".to_owned(), }; Error { + cause: format!( + "Found argument '{}' which wasn't expected, or isn't valid in this context{}", + a, did_you_mean_message + ), message: format!( "{} Found argument '{}' which wasn't expected, or isn't valid in \ this context{}\ @@ -886,6 +955,7 @@ impl Error { when: color, }); Error { + cause: e.description().to_string(), message: format!("{} {}", c.error("error:"), e.description()), kind: ErrorKind::Io, info: None, @@ -903,6 +973,7 @@ impl Error { when: ColorWhen::Auto, }); Error { + cause: format!("The argument '{}' wasn't found", a), message: format!( "{} The argument '{}' wasn't found", c.error("error:"), @@ -923,6 +994,7 @@ impl Error { when: ColorWhen::Auto, }); Error { + cause: description.to_string(), message: format!("{} {}", c.error("error:"), description), kind, info: None, diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 4334faba..e5b40cae 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -437,7 +437,9 @@ where needs_val_of ); match needs_val_of { - ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => continue, + ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => { + continue + } _ => (), } } else if arg_os.starts_with(b"-") && arg_os.len() != 1 { @@ -463,7 +465,9 @@ where )); } } - ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => continue, + ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => { + continue + } _ => (), } } @@ -700,6 +704,7 @@ where let mut out = vec![]; self.write_help_err(&mut out)?; return Err(ClapError { + cause: String::new(), message: String::from_utf8_lossy(&*out).into_owned(), kind: ErrorKind::MissingArgumentOrSubcommand, info: None, @@ -1534,6 +1539,7 @@ where match Help::new(&mut buf, self, use_long, false).write_help() { Err(e) => e, _ => ClapError { + cause: String::new(), message: String::from_utf8(buf).unwrap_or_default(), kind: ErrorKind::HelpDisplayed, info: None, @@ -1548,6 +1554,7 @@ where match self.print_version(&mut buf_w, use_long) { Err(e) => e, _ => ClapError { + cause: String::new(), message: String::new(), kind: ErrorKind::VersionDisplayed, info: None, @@ -1561,44 +1568,26 @@ impl<'b, 'c> Parser<'b, 'c> where 'b: 'c, { - fn contains_short(&self, s: char) -> bool { - self.app.contains_short(s) - } + fn contains_short(&self, s: char) -> bool { self.app.contains_short(s) } #[cfg_attr(feature = "lints", allow(needless_borrow))] - pub(crate) fn has_args(&self) -> bool { - self.app.has_args() - } + pub(crate) fn has_args(&self) -> bool { self.app.has_args() } - pub(crate) fn has_opts(&self) -> bool { - self.app.has_opts() - } + pub(crate) fn has_opts(&self) -> bool { self.app.has_opts() } - pub(crate) fn has_flags(&self) -> bool { - self.app.has_flags() - } + pub(crate) fn has_flags(&self) -> bool { self.app.has_flags() } pub(crate) fn has_positionals(&self) -> bool { self.app.args.keys.iter().any(|x| x.key.is_position()) } - pub(crate) fn has_subcommands(&self) -> bool { - self.app.has_subcommands() - } + pub(crate) fn has_subcommands(&self) -> bool { self.app.has_subcommands() } - pub(crate) fn has_visible_subcommands(&self) -> bool { - self.app.has_visible_subcommands() - } + pub(crate) fn has_visible_subcommands(&self) -> bool { self.app.has_visible_subcommands() } - pub(crate) fn is_set(&self, s: AS) -> bool { - self.app.is_set(s) - } + pub(crate) fn is_set(&self, s: AS) -> bool { self.app.is_set(s) } - pub(crate) fn set(&mut self, s: AS) { - self.app.set(s) - } + pub(crate) fn set(&mut self, s: AS) { self.app.set(s) } - pub(crate) fn unset(&mut self, s: AS) { - self.app.unset(s) - } + pub(crate) fn unset(&mut self, s: AS) { self.app.unset(s) } } diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 973155b0..ddef7ae1 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -67,6 +67,7 @@ impl<'b, 'c, 'z> Validator<'b, 'c, 'z> { let mut out = vec![]; self.p.write_help_err(&mut out)?; return Err(Error { + cause: String::new(), message: String::from_utf8_lossy(&*out).into_owned(), kind: ErrorKind::MissingArgumentOrSubcommand, info: None, @@ -398,14 +399,6 @@ impl<'b, 'c, 'z> Validator<'b, 'c, 'z> { } else { ma.vals.len() }, - if ma.vals.len() == 1 - || (a.is_set(ArgSettings::MultipleValues) - && (ma.vals.len() % num as usize) == 1) - { - "as" - } else { - "ere" - }, &*Usage::new(self.p).create_usage_with_title(&[]), self.p.app.color(), )); From 991b8390693ed93742bb4c766b9c3641582d8641 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Campinas?= Date: Thu, 31 Oct 2019 00:49:16 +0100 Subject: [PATCH 2/3] refactor(Error): add cause field to group_conflict error --- src/parse/errors.rs | 11 ++++++++++- src/parse/parser.rs | 44 +++++++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 59d6dda4..fd4ec347 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -424,7 +424,7 @@ impl Error { color: ColorWhen, ) -> Self where - O: Into, + O: Into + Copy, U: Display, { let mut v = vec![group.name.to_owned()]; @@ -433,6 +433,14 @@ impl Error { when: color, }); Error { + cause: format!( + "The argument '{}' cannot be used with {}", + group.name, + other.map_or( + "one or more of the other specified arguments".to_owned(), + |name| format!("'{}'", name.into()) + ) + ), message: format!( "{} The argument '{}' cannot be used with {}\n\n\ {}\n\n\ @@ -454,6 +462,7 @@ impl Error { info: Some(v), } } + #[doc(hidden)] pub fn argument_conflict(arg: &Arg, other: Option, usage: U, color: ColorWhen) -> Self where diff --git a/src/parse/parser.rs b/src/parse/parser.rs index e5b40cae..95dd90e0 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -437,9 +437,7 @@ where needs_val_of ); match needs_val_of { - ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => { - continue - } + ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => continue, _ => (), } } else if arg_os.starts_with(b"-") && arg_os.len() != 1 { @@ -465,9 +463,7 @@ where )); } } - ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => { - continue - } + ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => continue, _ => (), } } @@ -1568,26 +1564,44 @@ impl<'b, 'c> Parser<'b, 'c> where 'b: 'c, { - fn contains_short(&self, s: char) -> bool { self.app.contains_short(s) } + fn contains_short(&self, s: char) -> bool { + self.app.contains_short(s) + } #[cfg_attr(feature = "lints", allow(needless_borrow))] - pub(crate) fn has_args(&self) -> bool { self.app.has_args() } + pub(crate) fn has_args(&self) -> bool { + self.app.has_args() + } - pub(crate) fn has_opts(&self) -> bool { self.app.has_opts() } + pub(crate) fn has_opts(&self) -> bool { + self.app.has_opts() + } - pub(crate) fn has_flags(&self) -> bool { self.app.has_flags() } + pub(crate) fn has_flags(&self) -> bool { + self.app.has_flags() + } pub(crate) fn has_positionals(&self) -> bool { self.app.args.keys.iter().any(|x| x.key.is_position()) } - pub(crate) fn has_subcommands(&self) -> bool { self.app.has_subcommands() } + pub(crate) fn has_subcommands(&self) -> bool { + self.app.has_subcommands() + } - pub(crate) fn has_visible_subcommands(&self) -> bool { self.app.has_visible_subcommands() } + pub(crate) fn has_visible_subcommands(&self) -> bool { + self.app.has_visible_subcommands() + } - pub(crate) fn is_set(&self, s: AS) -> bool { self.app.is_set(s) } + pub(crate) fn is_set(&self, s: AS) -> bool { + self.app.is_set(s) + } - pub(crate) fn set(&mut self, s: AS) { self.app.set(s) } + pub(crate) fn set(&mut self, s: AS) { + self.app.set(s) + } - pub(crate) fn unset(&mut self, s: AS) { self.app.unset(s) } + pub(crate) fn unset(&mut self, s: AS) { + self.app.unset(s) + } } From 4d69942db797f64adc2f12a5ea57c9b3bbafa9cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Campinas?= Date: Sat, 2 Nov 2019 00:58:41 +0100 Subject: [PATCH 3/3] fix(Error): remove Copy trait bound by formatting both plain and colored error cause at the same time --- src/parse/errors.rs | 85 +++++++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index fd4ec347..eb31f648 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -424,7 +424,7 @@ impl Error { color: ColorWhen, ) -> Self where - O: Into + Copy, + O: Into, U: Display, { let mut v = vec![group.name.to_owned()]; @@ -432,29 +432,37 @@ impl Error { use_stderr: true, when: color, }); - Error { - cause: format!( - "The argument '{}' cannot be used with {}", - group.name, - other.map_or( - "one or more of the other specified arguments".to_owned(), - |name| format!("'{}'", name.into()) + let (plain_cause, colored_cause) = match other { + Some(name) => { + let n = name.into(); + v.push(n.clone()); + ( + format!("The argument '{}' cannot be used with '{}'", group.name, n), + format!( + "The argument '{}' cannot be used with '{}'", + group.name, + c.warning(n) + ), ) - ), + } + None => { + let n = "one or more of the other specified arguments"; + ( + format!("The argument '{}' cannot be used with {}", group.name, n), + format!( + "The argument '{}' cannot be used with {}", + group.name, + c.none(n) + ), + ) + } + }; + Error { + cause: plain_cause, message: format!( - "{} The argument '{}' cannot be used with {}\n\n\ - {}\n\n\ - For more information try {}", + "{} {}\n\n{}\n\nFor more information try {}", c.error("error:"), - c.warning(group.name), - match other { - Some(name) => { - let n = name.into(); - v.push(n.clone()); - c.warning(format!("'{}'", n)) - } - None => c.none("one or more of the other specified arguments".to_owned()), - }, + colored_cause, usage, c.good("--help") ), @@ -474,26 +482,37 @@ impl Error { use_stderr: true, when: color, }); - let conflicting_arg = match other { + let (plain_cause, colored_cause) = match other { Some(name) => { let n = name.into(); v.push(n.clone()); - c.warning(format!("'{}'", n)) + ( + format!("The argument '{}' cannot be used with '{}'", arg, n), + format!( + "The argument '{}' cannot be used with {}", + c.warning(arg.to_string()), + c.warning(format!("'{}'", n)) + ), + ) + } + None => { + let n = "one or more of the other specified arguments"; + ( + format!("The argument '{}' cannot be used with {}", arg, n), + format!( + "The argument '{}' cannot be used with {}", + c.warning(arg.to_string()), + c.none(n) + ), + ) } - None => c.none("one or more of the other specified arguments".to_owned()), }; Error { - cause: format!( - "The argument '{}' cannot be used with {}", - arg, conflicting_arg - ), + cause: plain_cause, message: format!( - "{} The argument '{}' cannot be used with {}\n\n\ - {}\n\n\ - For more information try {}", + "{} {}\n\n{}\n\nFor more information try {}", c.error("error:"), - c.warning(&*arg.to_string()), - conflicting_arg, + colored_cause, usage, c.good("--help") ),