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

List valid options in error messages #382

Merged
merged 2 commits into from
Jan 5, 2022

Conversation

dduan
Copy link
Contributor

@dduan dduan commented Jan 1, 2022

When an option value fails to parse, no custom error message is
provided, and a list of valid candidate values is available, include the
list as part of the error message.

Addresses #344.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@dduan
Copy link
Contributor Author

dduan commented Jan 1, 2022

@swift-ci Please test

@dduan
Copy link
Contributor Author

dduan commented Jan 1, 2022

Not holding any strong opinions about the specific wording. Suggestions welcome.

@natecook1000
Copy link
Member

This is great, @dduan!

I want to make sure this doesn't fall apart if allCases is more than a handful of cases. Can we switch to a bulleted list format in that case? Do you think 6 would make a good upper limit on the comma-delimited list?

What do you think of this for the output?

The value 'png' is invalid for '--format <format>'. Please provide one of 'text', 'json', or 'csv'.

The value 'png' is invalid for '--format <format>'. Please provide one of the following:
  - text
  - json
  - csv
  - plist
  - yaml
  - xaml
  - bson
  - xml

@rauhul
Copy link
Contributor

rauhul commented Jan 2, 2022

nit: for array arguments with non-empty allValues the help message should read "one or more of"

@dduan
Copy link
Contributor Author

dduan commented Jan 3, 2022

the help message should read "one or more of"

I've considered this suggestion and found it confusing in context.

  1. While it's true that the some parsing strategies allow --format text json, it's also possible that the list is supplied separately like --format text --format json.
  2. The beginning of the error message points to a single erroneous value: The value 'png' is invalid for....

IMO it's more accurate to suggest alternatives for a singular value, namely, the attempted value from the user's input.

When an option value fails to parse, no custom error message is
provided, and a list of valid candidate values is available, include the
list as part of the error message.

Addresses apple#344.
@dduan dduan force-pushed the dd/enumeratable-errorism branch from 4622806 to 8a339af Compare January 4, 2022 01:12
@dduan dduan force-pushed the dd/enumeratable-errorism branch from 8a339af to e2cd3f3 Compare January 4, 2022 01:22
@dduan
Copy link
Contributor Author

dduan commented Jan 4, 2022

@swift-ci test

@dduan
Copy link
Contributor Author

dduan commented Jan 4, 2022

I've implemented @natecook1000's suggestion.

enum Name: String, Equatable, Decodable, ExpressibleByArgument, CaseIterable {
case bruce
case clint
case hulk
Copy link
Member

Choose a reason for hiding this comment

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

Error: duplicate case detected 😜

@natecook1000
Copy link
Member

Looks great, thanks @dduan!

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.

3 participants