From d48c38d4278c5b4eb76891731b789c48a9642424 Mon Sep 17 00:00:00 2001 From: Vadim Frolov Date: Tue, 2 Mar 2021 21:33:26 +0100 Subject: [PATCH 1/3] Fix incorrect error message for arguments with choices - 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. --- knack/parser.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/knack/parser.py b/knack/parser.py index 4013b77..9aebc90 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -265,6 +265,8 @@ def _check_value(self, action, value): import difflib import sys + super(CLICommandParser, self)._check_value(action, value) + if action.choices is not None and value not in action.choices: # parser has no `command_source`, value is part of command itself error_msg = "{prog}: '{value}' is not in the '{prog}' command group. See '{prog} --help'.".format( From e26fd7470bcbdb143441133adbe6eea38331e183 Mon Sep 17 00:00:00 2001 From: Vadim Frolov Date: Tue, 2 Mar 2021 23:15:17 +0100 Subject: [PATCH 2/3] Make error message about invalid choice consistent with other Knack error messages - This seems to be a better fix for the previous commit. --- knack/parser.py | 47 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/knack/parser.py b/knack/parser.py index 9aebc90..bcfb504 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -30,6 +30,20 @@ ] +def _get_action_name(argument): + # copied from argparse in order not to use a protected method + if argument is None: + return None + if argument.option_strings: + return '/'.join(argument.option_strings) + if argument.metavar not in (None, argparse.SUPPRESS): + return argument.metavar + if argument.dest not in (None, argparse.SUPPRESS): + return argument.dest + + return None + + class CLICommandParser(argparse.ArgumentParser): @staticmethod @@ -265,22 +279,35 @@ def _check_value(self, action, value): import difflib import sys - super(CLICommandParser, self)._check_value(action, value) - if action.choices is not None and value not in action.choices: - # parser has no `command_source`, value is part of command itself - error_msg = "{prog}: '{value}' is not in the '{prog}' command group. See '{prog} --help'.".format( - prog=self.prog, value=value) - logger.error(error_msg) - candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) + is_command = action.dest in ["_command", "_subcommand"] + candidates = action.choices + if is_command: + candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) + if candidates: print_args = { 's': 's' if len(candidates) > 1 else '', 'verb': 'are' if len(candidates) > 1 else 'is', - 'value': value + 'value': value, + 'action': _get_action_name(action), + 'choices': ', '.join(map(repr, candidates)) } - suggestion_msg = "\nThe most similar choice{s} to '{value}' {verb}:\n".format(**print_args) + + if is_command: + # parser has no `command_source`, value is part of command itself + error_msg = "{prog}: '{value}' is not in the '{prog}' command group. See '{prog} --help'.".format( + prog=self.prog, value=value) + logger.error(error_msg) + if candidates: + suggestion_msg = "\nThe most similar choice{s} to '{value}' {verb}:\n".format(**print_args) + suggestion_msg += '\n'.join(['\t' + candidate for candidate in candidates]) + print(suggestion_msg, file=sys.stderr) + else: + error_msg = "{prog}: invalid choice '{value}'. See '{prog} --help'.".format( + prog=self.prog, value=value) + logger.error(error_msg) + suggestion_msg = "\nPossible choice{s} for argument '{action}' {verb}:\n".format(**print_args) suggestion_msg += '\n'.join(['\t' + candidate for candidate in candidates]) print(suggestion_msg, file=sys.stderr) - self.exit(2) From 639f16e141cb654ba116b3f78cc53df4bf3f5e46 Mon Sep 17 00:00:00 2001 From: Vadim Frolov Date: Tue, 2 Mar 2021 23:30:12 +0100 Subject: [PATCH 3/3] Pass flake --- knack/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/knack/parser.py b/knack/parser.py index bcfb504..e79b744 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -35,7 +35,7 @@ def _get_action_name(argument): if argument is None: return None if argument.option_strings: - return '/'.join(argument.option_strings) + return '/'.join(argument.option_strings) if argument.metavar not in (None, argparse.SUPPRESS): return argument.metavar if argument.dest not in (None, argparse.SUPPRESS):