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

fix(derive)!: Don't Panic, Error #2943

Merged
merged 5 commits into from Oct 26, 2021
Merged

fix(derive)!: Don't Panic, Error #2943

merged 5 commits into from Oct 26, 2021

Conversation

epage
Copy link
Member

@epage epage commented Oct 25, 2021

This switches all of our unwrap and expects in the generated code to pass errors up.

#2890 exposed App::error to users. While this PR was the encouragement for it, I felt it was a worthwhile addition to the API anyways. For most users, it will be sufficient but unfortunately not for the derive. We don't have access to the App at the error site. We need to pass up the error in "raw" form and then format it right at the end. To accomplish this, we are now tracking whether the error is formatted or not and expose to users Error::raw to create an unformatted error and Error::format to then format it. Parser inherent methods were updated to do this formatting for the user.

To abstract the difference in formatting, I used Cow. To support Cow, I needed to make some internal types Cloneable.

Fixes #2255

This gives us room to add extra context later.
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.
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.
The derive is generating `Error::raw` (from scratch or by converting
existing erors) and then the inherent `Parser` methods format them.

Fixes clap-rs#2255
@epage epage added the M-unreviewed Meta: Request for post-merge review. label Oct 26, 2021
@epage
Copy link
Member Author

epage commented Oct 26, 2021

Going ahead with this as I've got some more branches blocked on this

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 26, 2021

Build succeeded:

@bors bors bot merged commit 07fcaa9 into clap-rs:master Oct 26, 2021
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Looks good but I wish you had waited on this for a little bit more time because this is an impactful change.

@pksunkara pksunkara removed the M-unreviewed Meta: Request for post-merge review. label Oct 26, 2021
@pksunkara
Copy link
Member

pksunkara commented Oct 26, 2021

Also, needs tests.

@epage epage deleted the derive_error branch October 26, 2021 20:58
@epage
Copy link
Member Author

epage commented Oct 26, 2021

Looks good but I wish you had waited on this for a little bit more time because this is an impactful change.

With each change, I have to balance how much more work I can do on top of it vs sit idle. With higher chances of conflicts (like this PR), the chance of me sitting idle is higher so I can be responsive to change. A counter-balance to keep things progressing is lower risk changes (e.g. things that aren't direction setting like #2617), since I can just as easily respond to review feedback in distinct PRs as not and they will most likely have a lower chance of conflicts.

epage added a commit to epage/clap that referenced this pull request Oct 29, 2021
This is to help verify behavior added in clap-rs#2943.  We separated the error
raising site from the error formatting site and this verifies that the
formatting actually happens.
epage added a commit to epage/clap that referenced this pull request Oct 29, 2021
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.

This is a follow up to clap-rs#2943; apparently I had missed some cases.
@mzabaluev
Copy link

Now that it's possible to produce validation errors while parsing the values from ArgMatches, could the derive macros be reworked to not parse the values another time, only to drop the value?

@epage
Copy link
Member Author

epage commented Nov 22, 2021

The error reporting in this PR is much more crude than what the validators can perform. #2683 will remove the double parse generally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppSettings::SubcommandsNegateReqs may cause panic in Clap::parse(), Clap::try_parse()
3 participants