Skip to content
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

feat: Expose clap-style errors to users #2890

Merged
merged 1 commit into from
Oct 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/build/app/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::{
output::{fmt::Colorizer, Help, HelpWriter, Usage},
parse::{ArgMatcher, ArgMatches, Input, Parser},
util::{color::ColorChoice, safe_exit, Id, Key, USAGE_CODE},
Result as ClapResult, INTERNAL_ERROR_MSG,
Error, ErrorKind, Result as ClapResult, INTERNAL_ERROR_MSG,
};

/// Represents a command line interface which is made up of all possible
Expand Down Expand Up @@ -1801,6 +1801,21 @@ impl<'help> App<'help> {
self
}

/// Custom error message for post-parsing validation
///
/// # Examples
///
/// ```rust
/// # use clap::{App, ErrorKind};
/// let mut app = App::new("myprog");
/// let err = app.error(ErrorKind::InvalidValue, "Some failure case");
/// ```
pub fn error(&mut self, kind: ErrorKind, message: impl std::fmt::Display) -> Error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I am not completely sold on this API. Do you have a rough sketch on how this would be used in clap_derive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In clap_derive, I expect to do something like

let mut app = <Self as IntoApp>::into_app();
return Err(app.error(ErrorKind::MissingSubcommand, "some message"));

What is it about it that you are concerned about?

I chose impl std::fmt::Display as that would easily allow another error to be turned into a clap error message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if by "how", you don't mean the call but higher level, this is part of turning our unwraps into errors. Most of these will just be about passing errors further up the stack. Some will be error conversion. In one case, its to handle when a subcommand is required but not present. We expect get_matches to validate all of these cases but

  • There are corner cases of user settings that cause this to break down
  • If a builder user wants to reuse a derive code (e.g. cargo using clap-cargo), then they will probably want errors to help them through misuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how do you expect this to be used in post-parsing? I think what we need there is to render usage based on args given (style of clap errors). Not from a fresh app.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, showing the general usage rather than the specific usage is another compromise this makes.

It looks like we do this semi-regularly with our existing errors (passing Usage::new(self.p).create_usage_with_title(&[])).

Requiring support for populating the used at minimum complicates this to where it might lose value to the user and at worse it makes it impossible to provide this and yet we have use cases and needs for it.

self._build();
let usage = self.render_usage();
Error::user_error(self, usage, kind, message)
}

/// Prints the full help message to [`io::stdout()`] using a [`BufWriter`] using the same
/// method as if someone ran `-h` to request the help message.
///
Expand Down
48 changes: 35 additions & 13 deletions src/parse/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ pub enum ErrorKind {
}

/// Command Line Argument Parser Error
///
/// See [`App::error`] to create an error.
///
/// [`App::error`]: crate::App::error
#[derive(Debug)]
pub struct Error {
/// Formatted error message, enhancing the cause message with extra information
Expand Down Expand Up @@ -540,6 +544,27 @@ impl Error {
}
}

pub(crate) fn user_error(
app: &App,
usage: String,
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 {
message: c,
kind,
info: vec![],
source: None,
backtrace: Backtrace::new(),
}
}

pub(crate) fn argument_conflict(
app: &App,
arg: &Arg,
Expand Down Expand Up @@ -1077,34 +1102,31 @@ impl Error {
}
}

/// Create an error with a custom description.
/// Deprecated, see [`App::error`]
///
/// This can be used in combination with `Error::exit` to exit your program
/// with a custom error message.
/// [`App::error`]: crate::App::error
#[deprecated(since = "3.0.0", note = "Replaced with `App::error`")]
pub fn with_description(description: String, kind: ErrorKind) -> Self {
epage marked this conversation as resolved.
Show resolved Hide resolved
let mut c = Colorizer::new(true, ColorChoice::Auto);

start_error(&mut c, description);

Error {
message: c,
kind,
info: vec![],
source: None,
backtrace: Backtrace::new(),
}
Error::new(c, kind)
}
}

impl From<io::Error> for Error {
fn from(e: io::Error) -> Self {
Error::with_description(e.to_string(), ErrorKind::Io)
let mut c = Colorizer::new(true, ColorChoice::Auto);
start_error(&mut c, e.to_string());
Error::new(c, ErrorKind::Io)
}
}

impl From<fmt::Error> for Error {
fn from(e: fmt::Error) -> Self {
Error::with_description(e.to_string(), ErrorKind::Format)
let mut c = Colorizer::new(true, ColorChoice::Auto);
start_error(&mut c, e.to_string());
Error::new(c, ErrorKind::Format)
}
}

Expand Down
61 changes: 61 additions & 0 deletions tests/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
mod utils;

use clap::{App, Arg, ColorChoice, Error, ErrorKind};

fn compare_error(
err: Error,
expected_kind: ErrorKind,
expected_output: &str,
stderr: bool,
) -> bool {
let actual_output = err.to_string();
assert_eq!(
stderr,
err.use_stderr(),
"Should Use STDERR failed. Should be {} but is {}",
stderr,
err.use_stderr()
);
assert_eq!(expected_kind, err.kind);
utils::compare(expected_output, actual_output)
}

#[test]
fn app_error() {
static MESSAGE: &str = "error: Failed for mysterious reasons

USAGE:
test [OPTIONS] --all

For more information try --help
";
let app = App::new("test")
.arg(
Arg::new("all")
.short('a')
.long("all")
.required(true)
.about("Also do versioning for private crates (will not be published)"),
)
.arg(
Arg::new("exact")
.long("exact")
.about("Specify inter dependency version numbers exactly with `=`"),
)
.arg(
Arg::new("no_git_commit")
.long("no-git-commit")
.about("Do not commit version changes"),
)
.arg(
Arg::new("no_git_push")
.long("no-git-push")
.about("Do not push generated commit and tags to git remote"),
);
#[cfg(feature = "color")]
let app = app.color(ColorChoice::Never);
let mut app = app;
let expected_kind = ErrorKind::InvalidValue;
let err = app.error(expected_kind, "Failed for mysterious reasons");
assert!(compare_error(err, expected_kind, MESSAGE, true));
}
2 changes: 1 addition & 1 deletion tests/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use regex::Regex;

use clap::{App, Arg, ArgGroup};

fn compare<S, S2>(l: S, r: S2) -> bool
pub fn compare<S, S2>(l: S, r: S2) -> bool
where
S: AsRef<str>,
S2: AsRef<str>,
Expand Down