-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Enhance OptionParser.ParseError with available options display #14673
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
Conversation
Example output: Expected one of: --count INTEGER (alias: -c) --debug, --no-debug (alias: -d) --files STRING (alias: -f) (may be given more than once) --verbose, --no-verbose (alias: -v) Prompt ====== When we raise ParseError, include all of the options we could potentially accept, alongside their types and aliases. For example, the switches `[foo: :string, bar: :integer]` and `aliases: [b: :bar]`, the error message should say: Expected one of: --foo STRING --bar INTEGER (alias: -b) Furthermore, for types that are :keep (which default to string), you should add: --bar INTEGER (alias: -b) (may be given more than once) And boolean ones accept no arguments, so they should be written as: --baz, --no-baz Sort all of them alphabetically.
@@ -282,11 +282,11 @@ defmodule OptionParser do | |||
|
|||
iex> OptionParser.parse!(["--limit", "xyz"], strict: [limit: :integer]) | |||
** (OptionParser.ParseError) 1 error found! | |||
--limit : Expected type integer, got "xyz" | |||
--limit : Expected type integer, got "xyz"... |
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.
I had to add those manually.
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.
Should it really be printed in this case, though? I feel like it makes a lot of sense to print all the options when you use a unknown option, but when you use the wrong type, isn't it mostly noise?
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.
Agreed, it's probably more tokens for the LLM to consume too 😄 However, sometimes you might be doing --limit $ENV_VAR
and it could be useful to see what the env var was expanded as
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.
I think it should because the wrong type can also be an indicator that you are calling the wrong option, especially when aliases are involved. For example, you can pass -n with a name, thinking it is about --name, but it is actually a number.
Co-authored-by: rmand97 <[email protected]>
💚 💙 💜 💛 ❤️ |
Example output: Expected one of: --count INTEGER (alias: -c) --debug, --no-debug (alias: -d) --files STRING (alias: -f) (may be given more than once) --verbose, --no-verbose (alias: -v) Prompt ====== When we raise ParseError, include all of the options we could potentially accept, alongside their types and aliases. For example, the switches `[foo: :string, bar: :integer]` and `aliases: [b: :bar]`, the error message should say: Expected one of: --foo STRING --bar INTEGER (alias: -b) Furthermore, for types that are :keep (which default to string), you should add: --bar INTEGER (alias: -b) (may be given more than once) And boolean ones accept no arguments, so they should be written as: --baz, --no-baz Sort all of them alphabetically.
Example output:
Prompt
When we raise ParseError, include all of the options we could potentially accept, alongside their types and aliases. For example, the switches
[foo: :string, bar: :integer]
andaliases: [b: :bar]
, the error message should say:Furthermore, for types that are :keep (which default to string), you should add:
And boolean ones accept no arguments, so they should be written as:
Sort all of them alphabetically.