-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add error message for invalid argument value #244
Conversation
- If user provided an invalid value for an argument with a list of choices, then the error message will not indicate this. Rather it will indicate that cli command group is invalid. This is a quick fix and I am not sure that it doesn't break Knack. From my tests, this fix is better than removing overridden _check_value method.
…rror messages - This seems to be a better fix for the previous commit.
# Show suggestions | ||
candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) | ||
if candidates: | ||
suggestion_msg = "\nThe most similar choices to '{value}':\n".format(value=value) |
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.
No need to play around with English grammar for third person singular form.
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.
👍
name=argparse._get_action_name(action)) # pylint: disable=protected-access | ||
logger.error(error_msg) | ||
# Show all allowed values | ||
suggestion_msg = "Allowed values: " + ', '.join(action.choices) |
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.
The number of allowed argument values is usually very limited. Show all of them.
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.
👍
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.
Looks good to me!
# Show suggestions | ||
candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) | ||
if candidates: | ||
suggestion_msg = "\nThe most similar choices to '{value}':\n".format(value=value) |
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.
👍
name=argparse._get_action_name(action)) # pylint: disable=protected-access | ||
logger.error(error_msg) | ||
# Show all allowed values | ||
suggestion_msg = "Allowed values: " + ', '.join(action.choices) |
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.
👍
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.
LGTM
Fix #236
Migrated from #237
Symptom
Knack can only give error message for invalid command, but not invalid argument value.
History: #237 (comment)
Change
Add error message for invalid argument value.
Testing guide
ℹ As
Allowed values: rock, paper, scissors
appears in one line, it is not needed to add an empty line before it.