From 21cc39678b7680029bb2179e5eb283700ac8521d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 29 Oct 2021 16:26:34 -0500 Subject: [PATCH] fix(derive): Support WaitOnError for derive-genned errors Before, `Error::exit` didn't provide `WaitOnError`, requiring each call site to duplicate half of `Error::exit`s behavior to get it. This hadn't been done for errors raised by derive-generated code. Ideally, these errors never happen but all the same, having this consistent would be good. This moves knowledge of `WaitOnError` to `Error` (including through `Error::format`) so `Error::exit` can wait. Now all of the callers to `.exit` get a consistent experience without duplication. While #2938 made a lot of `Error` fields optional for less churn-heavy modifications, I made this new field required to minimize the risk of an raise site forgetting to set it. --- src/build/app/mod.rs | 39 +---------- src/parse/errors.rs | 148 +++++++++++++++++++++++++++++++++-------- src/parse/parser.rs | 13 +++- src/parse/validator.rs | 1 + 4 files changed, 137 insertions(+), 64 deletions(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 40761860..9c9383df 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -12,7 +12,7 @@ use std::{ env, ffi::OsString, fmt, - io::{self, BufRead, Write}, + io::{self, Write}, ops::Index, path::Path, }; @@ -28,7 +28,7 @@ use crate::{ mkeymap::MKeyMap, output::{fmt::Colorizer, Help, HelpWriter, Usage}, parse::{ArgMatcher, ArgMatches, Input, Parser, ValueType}, - util::{color::ColorChoice, safe_exit, Id, Key, USAGE_CODE}, + util::{color::ColorChoice, Id, Key}, Error, ErrorKind, Result as ClapResult, INTERNAL_ERROR_MSG, }; @@ -2029,24 +2029,7 @@ impl<'help> App<'help> { /// [`App::get_matches`]: App::get_matches() pub fn get_matches_mut(&mut self) -> ArgMatches { self.try_get_matches_from_mut(&mut env::args_os()) - .unwrap_or_else(|e| { - // Otherwise, write to stderr and exit - if e.use_stderr() { - e.print().expect("Error writing Error to stderr"); - - if self.settings.is_set(AppSettings::WaitOnError) { - wlnerr!("\nPress [ENTER] / [RETURN] to continue..."); - let mut s = String::new(); - let i = io::stdin(); - i.lock().read_line(&mut s).unwrap(); - } - - drop(e); - safe_exit(USAGE_CODE); - } - - e.exit() - }) + .unwrap_or_else(|e| e.exit()) } /// Starts the parsing process. This method will return a [`clap::Result`] type instead of exiting @@ -2112,22 +2095,6 @@ impl<'help> App<'help> { T: Into + Clone, { self.try_get_matches_from_mut(itr).unwrap_or_else(|e| { - // Otherwise, write to stderr and exit - if e.use_stderr() { - e.print().expect("Error writing Error to stderr"); - - if self.settings.is_set(AppSettings::WaitOnError) { - wlnerr!("\nPress [ENTER] / [RETURN] to continue..."); - let mut s = String::new(); - let i = io::stdin(); - i.lock().read_line(&mut s).unwrap(); - } - - drop(self); - drop(e); - safe_exit(USAGE_CODE); - } - drop(self); e.exit() }) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 93b46e51..2fc5900c 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -4,7 +4,7 @@ use std::{ convert::From, error, fmt::{self, Debug, Display, Formatter}, - io, + io::{self, BufRead}, result::Result as StdResult, }; @@ -438,6 +438,7 @@ pub struct Error { /// Useful when you want to render an error of your own. pub info: Vec, pub(crate) source: Option>, + wait_on_exit: bool, backtrace: Backtrace, } @@ -498,7 +499,7 @@ impl Error { /// /// [`App::error`]: crate::App::error pub fn raw(kind: ErrorKind, message: impl std::fmt::Display) -> Self { - Self::new(message.to_string(), kind) + Self::new(message.to_string(), kind, false) } /// Format the existing message with the App's context @@ -506,6 +507,7 @@ impl Error { app._build(); let usage = app.render_usage(); self.message.format(app, usage); + self.wait_on_exit = app.settings.is_set(AppSettings::WaitOnError); self } @@ -525,6 +527,14 @@ impl Error { if self.use_stderr() { // Swallow broken pipe errors let _ = self.print(); + + if self.wait_on_exit { + wlnerr!("\nPress [ENTER] / [RETURN] to continue..."); + let mut s = String::new(); + let i = io::stdin(); + i.lock().read_line(&mut s).unwrap(); + } + safe_exit(USAGE_CODE); } @@ -553,12 +563,13 @@ impl Error { self.message.formatted().print() } - pub(crate) fn new(message: impl Into, kind: ErrorKind) -> Self { + pub(crate) fn new(message: impl Into, kind: ErrorKind, wait_on_exit: bool) -> Self { Self { message: message.into(), kind, info: vec![], source: None, + wait_on_exit, backtrace: Backtrace::new(), } } @@ -605,7 +616,12 @@ impl Error { info.push(other); } - Self::new(c, ErrorKind::ArgumentConflict).set_info(info) + Self::new( + c, + ErrorKind::ArgumentConflict, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(info) } pub(crate) fn empty_value(app: &App, arg: &Arg, usage: String) -> Self { @@ -618,7 +634,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::EmptyValue).set_info(vec![arg]) + Self::new( + c, + ErrorKind::EmptyValue, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg]) } pub(crate) fn no_equals(app: &App, arg: String, usage: String) -> Self { @@ -631,7 +652,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::NoEquals).set_info(vec![arg]) + Self::new( + c, + ErrorKind::NoEquals, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg]) } pub(crate) fn invalid_value( @@ -689,7 +715,12 @@ impl Error { let mut info = vec![arg.to_string(), bad_val]; info.extend(sorted); - Self::new(c, ErrorKind::InvalidValue).set_info(info) + Self::new( + c, + ErrorKind::InvalidValue, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(info) } pub(crate) fn invalid_subcommand( @@ -715,7 +746,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::InvalidSubcommand).set_info(vec![subcmd]) + Self::new( + c, + ErrorKind::InvalidSubcommand, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![subcmd]) } pub(crate) fn unrecognized_subcommand(app: &App, subcmd: String, name: String) -> Self { @@ -728,7 +764,12 @@ impl Error { c.none(format!("\n {} ", name)); try_help(app, &mut c); - Self::new(c, ErrorKind::UnrecognizedSubcommand).set_info(vec![subcmd]) + Self::new( + c, + ErrorKind::UnrecognizedSubcommand, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![subcmd]) } pub(crate) fn missing_required_argument( @@ -753,7 +794,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::MissingRequiredArgument).set_info(info) + Self::new( + c, + ErrorKind::MissingRequiredArgument, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(info) } pub(crate) fn missing_subcommand(app: &App, name: String, usage: String) -> Self { @@ -765,7 +811,11 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::MissingSubcommand) + Self::new( + c, + ErrorKind::MissingSubcommand, + app.settings.is_set(AppSettings::WaitOnError), + ) } pub(crate) fn invalid_utf8(app: &App, usage: String) -> Self { @@ -778,7 +828,11 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::InvalidUtf8) + Self::new( + c, + ErrorKind::InvalidUtf8, + app.settings.is_set(AppSettings::WaitOnError), + ) } pub(crate) fn too_many_occurrences( @@ -801,7 +855,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::TooManyOccurrences).set_info(vec![ + Self::new( + c, + ErrorKind::TooManyOccurrences, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![ arg.to_string(), curr_occurs.to_string(), max_occurs.to_string(), @@ -819,7 +878,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::TooManyValues).set_info(vec![arg, val]) + Self::new( + c, + ErrorKind::TooManyValues, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg, val]) } pub(crate) fn too_few_values( @@ -842,7 +906,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::TooFewValues).set_info(vec![ + Self::new( + c, + ErrorKind::TooFewValues, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![ arg.to_string(), curr_vals.to_string(), min_vals.to_string(), @@ -855,7 +924,13 @@ impl Error { val: String, err: Box, ) -> Self { - let mut err = Self::value_validation_with_color(arg, val, err, app.get_color()); + let mut err = Self::value_validation_with_color( + arg, + val, + err, + app.get_color(), + app.settings.is_set(AppSettings::WaitOnError), + ); match &mut err.message { Message::Raw(_) => { unreachable!("`value_validation_with_color` only deals in formatted errors") @@ -870,7 +945,7 @@ impl Error { val: String, err: Box, ) -> Self { - Self::value_validation_with_color(arg, val, err, ColorChoice::Never) + Self::value_validation_with_color(arg, val, err, ColorChoice::Never, false) } fn value_validation_with_color( @@ -878,6 +953,7 @@ impl Error { val: String, err: Box, color: ColorChoice, + wait_on_exit: bool, ) -> Self { let mut c = Colorizer::new(true, color); @@ -889,7 +965,7 @@ impl Error { c.none(format!(": {}", err)); - Self::new(c, ErrorKind::ValueValidation) + Self::new(c, ErrorKind::ValueValidation, wait_on_exit) .set_info(vec![arg, val, err.to_string()]) .set_source(err) } @@ -914,7 +990,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::WrongNumberOfValues).set_info(vec![ + Self::new( + c, + ErrorKind::WrongNumberOfValues, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![ arg.to_string(), curr_vals.to_string(), num_vals.to_string(), @@ -931,7 +1012,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::UnexpectedMultipleUsage).set_info(vec![arg]) + Self::new( + c, + ErrorKind::UnexpectedMultipleUsage, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg]) } pub(crate) fn unknown_argument( @@ -975,7 +1061,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::UnknownArgument).set_info(vec![arg]) + Self::new( + c, + ErrorKind::UnknownArgument, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg]) } pub(crate) fn unnecessary_double_dash(app: &App, arg: String, usage: String) -> Self { @@ -992,7 +1083,12 @@ impl Error { put_usage(&mut c, usage); try_help(app, &mut c); - Self::new(c, ErrorKind::UnknownArgument).set_info(vec![arg]) + Self::new( + c, + ErrorKind::UnknownArgument, + app.settings.is_set(AppSettings::WaitOnError), + ) + .set_info(vec![arg]) } pub(crate) fn argument_not_found_auto(arg: String) -> Self { @@ -1002,7 +1098,7 @@ impl Error { c.warning(arg.clone()); c.none("' wasn't found"); - Self::new(c, ErrorKind::ArgumentNotFound).set_info(vec![arg]) + Self::new(c, ErrorKind::ArgumentNotFound, false).set_info(vec![arg]) } /// Deprecated, see [`App::error`] @@ -1010,19 +1106,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 { - Error::new(description, kind) + Error::raw(kind, description) } } impl From for Error { fn from(e: io::Error) -> Self { - Error::new(e.to_string(), ErrorKind::Io) + Error::raw(ErrorKind::Io, e) } } impl From for Error { fn from(e: fmt::Error) -> Self { - Error::new(e.to_string(), ErrorKind::Format) + Error::raw(ErrorKind::Format, e) } } diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 971b848a..69238702 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -763,6 +763,7 @@ impl<'help, 'app> Parser<'help, 'app> { return Err(ClapError::new( message, ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand, + self.app.settings.is_set(AS::WaitOnError), )); } @@ -1881,7 +1882,11 @@ impl<'help, 'app> Parser<'help, 'app> { match Help::new(HelpWriter::Buffer(&mut c), self, use_long).write_help() { Err(e) => e.into(), - _ => ClapError::new(c, ErrorKind::DisplayHelp), + _ => ClapError::new( + c, + ErrorKind::DisplayHelp, + self.app.settings.is_set(AS::WaitOnError), + ), } } @@ -1891,7 +1896,11 @@ impl<'help, 'app> Parser<'help, 'app> { let msg = self.app._render_version(use_long); let mut c = Colorizer::new(false, self.app.get_color()); c.none(msg); - ClapError::new(c, ErrorKind::DisplayVersion) + ClapError::new( + c, + ErrorKind::DisplayVersion, + self.app.settings.is_set(AS::WaitOnError), + ) } } diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 317b7c8d..f699ba4b 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -66,6 +66,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { return Err(Error::new( message, ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand, + self.p.is_set(AS::WaitOnError), )); } self.validate_conflicts(matcher)?;