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

Supplement clap::Error.info with helper methods #2628

Closed
2 tasks done
TheDaemoness opened this issue Jul 27, 2021 · 6 comments · Fixed by #3402
Closed
2 tasks done

Supplement clap::Error.info with helper methods #2628

TheDaemoness opened this issue Jul 27, 2021 · 6 comments · Fixed by #3402
Labels
A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-mentor Status: Needs elaboration on the details before doing a 'Call for participation'
Milestone

Comments

@TheDaemoness
Copy link
Contributor

TheDaemoness commented Jul 27, 2021

Maintainer's notes

  • Ideally we'd make dynamic member access the API for this but that is still going through the RFC process
  • If we want a jump start on that API, we could provide our own "any map" that allows attaching arbitrary enums to the error struct.
  • Overall, this avoids limiting ourselves to each error kind having an exact set of data in every case

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Clap Version

3.0.0-beta.2

Describe your use case

Preface: thank you very much for your work on clap so far, it has saved me countless hours of tedium.

ErrorKind is a fieldless enum, so additional contextual information required for someone to provide their own custom error messages is provided as a Vec<String> in Error, named info.

Using a Vec to carry this information not self-explanatory in regards to what each element of the Vec represents (e.g. which element represents the invalid value that triggers an InvalidValue or ValueValidation). This would be okay-ish if clap had any of the following:

  • Methods or associated functions on Error or ErrorKind to retrieve specific information
  • Functions, named constants, or an enum that can be used with info to retrieve specific information based on where it should be in the vector.
  • Documentation describing what the values of info will be for a given ErrorKind.

As far as I can tell, none of these are present, which means that in order to write one's own error reporting, one needs to either read the source code of the functions that construct these Errors or do their own testing. I don't think either should be necessary.

Describe the solution you'd like

The solution I'd prefer the most is a significant breaking change (described under Alternatives). I acknowledge that's not desirable, even considering the upcoming 3.0.0 release.

A compromise is to add ways to retrieve this information safely, such as by adding methods to Error that return the relevant information. A few possible examples with placeholder names:

  • pub fn get_arg_or_subcmd(&self) -> Option<&str>
  • pub fn get_missing(&self) -> &[String]
  • pub fn get_value_counts(&self) -> Option<(usize,usize)> //Found, expected.

Implementing each method should be fairly simple, if perhaps a bit repetitive considering the number of variants on ErrorKind that need to be matched. The bodies would mostly be pattern matches followed by accesses to info.

One caveat in the implementation is that Error should contain a private flag to indicate whether the Error was constructed by Error::with_description or not. If so, these functions should return empty values.

Alternatives, if applicable

This problem can technically be solved by improving documentation, but I'm of the mind that doing only that is the worst solution. info being a Vec is fragile and typo-prone, and there should be better ways to interact with it than as a Vec.

Ideally, keeping ErrorKind as a fieldless enum, info's type would change to an enum with fields that store the relevant information for each kind of error. Some of the methods described above could still be implemented for convenience.

However, I acknowledge that this would be a significant breaking change that would take more than a few renames for users of clap to fix. Solutions like making info a method that generates a Vec<String> from data in this enum or providing both info and the enum in another field aren't entirely compelling either.

Additional Context

I am very willing to work with you fine folks to implement any of these changes, if approved. Have a good day!

@TheDaemoness TheDaemoness changed the title Supplement crate::Error.info with helper methods Supplement clap::Error.info with helper methods Jul 27, 2021
@pksunkara pksunkara added this to the 3.0 milestone Aug 11, 2021
@pksunkara
Copy link
Member

@epage Do you think you can take care of this after the utf8 thing you are currently working on?

@pksunkara
Copy link
Member

If we are breaking it, I would actually love the variants in ErrorKind to be tuples of fields that can contain the metadata of the error.

@epage
Copy link
Member

epage commented Aug 11, 2021

After iterating with different error designs, I think our approach of a tag-only Kind with auxiliary data is the best approach (granted, how we expose that auxiliary data can be improved). The problem with enum fields is its brittle (can't add fields without breaking compat) and harder to check for just the kind (matches! helps some but not completely).

The error working group is working on a general API for querying information from errors. Something like that seems like the best long-term solution. Short term, we can add our own query methods if we really need this before the Error WG gets their feature landed.

@pksunkara
Copy link
Member

Error WG gets their feature landed.

Which feature is that?

we can add our own query methods

In that case, shall I mark this as non 3.0 since it wont be breaking change?

@epage
Copy link
Member

epage commented Aug 11, 2021

Error WG gets their feature landed.
Which feature is that?

Dynamic member access, rust-lang/rfcs#2895

The RFC is old but it looks like work has restarted on this. Just today they were talking about this on their Zulip channel.

In that case, shall I mark this as non 3.0 since it wont be breaking change?

Agreed

@pksunkara pksunkara removed this from the 3.0 milestone Aug 11, 2021
@epage epage added A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-mentor Status: Needs elaboration on the details before doing a 'Call for participation' and removed C: errors labels Dec 8, 2021
@epage epage added this to the 4.0 milestone Dec 14, 2021
epage added a commit to epage/clap that referenced this issue Feb 4, 2022
This is meant to replace `Error::info` as part of clap-rs#2628.
@epage
Copy link
Member

epage commented Feb 4, 2022

Mind taking a look at #3402 to see if it'd work?

epage added a commit to epage/clap that referenced this issue Feb 7, 2022
This is meant to replace `Error::info` as part of clap-rs#2628.
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
- ArgSettings are part of clap-rs#2717
- Errors are part of clap-rs#2628
- `help_heading` is part of clap-rs#1807 and clap-rs#1553
- Some misc parts are for API consistency
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
- ArgSettings are part of clap-rs#2717
- Errors are part of clap-rs#2628
- `help_heading` is part of clap-rs#1807 and clap-rs#1553
- Some misc parts are for API consistency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-mentor Status: Needs elaboration on the details before doing a 'Call for participation'
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants