-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need tests.
/// 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Added a test for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Build failed: |
This gives users the basic error template for quick and dirty messages. In addition to the lack of customization, they are not given anything to help them with coloring or for programmayic use (info, source). This is something I've wanted many times for one-off validation that can't be expressed with clap's validation or it just wasn't worth the hoops. The more pressing need is for clap-rs#2255, I need `clap_derive` to be able to create error messages and `Error::with_description` seemed too disjoint from the rest of the clap experience that it seemed like users would immediately create issues about it showing up. With this available, I've gone ahead and deprecated `Error::with_description` (added in 58512f2), assuming this will be sufficient for users needs (or they can use IO Errors as a back door). I did so according to the pattern in clap-rs#2718 despite us not being fully resolved on that approach yet.
bors r+ |
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
Build succeeded: |
This gives users the basic error template for quick and dirty messages.
In addition to the lack of customization, they are not given anything to help
them with coloring or for programmayic use (info, source).
This is something I've wanted many times for one-off validation that
can't be expressed with clap's validation or it just wasn't worth
the hoops. The more pressing need is for #2255, I need
clap_derive
to be able to create error messages and
Error::with_description
seemedtoo disjoint from the rest of the clap experience that it seemed like
users would immediately create issues about it showing up.
With this available, I've gone ahead and deprecated
Error::with_description
(added in 58512f2), assuming this will besufficient for users needs (or they can use IO Errors as a back door).
I did so according to the pattern in #2718 despite us not being fully
resolved on that approach yet.