From d48c38d4278c5b4eb76891731b789c48a9642424 Mon Sep 17 00:00:00 2001 From: Vadim Frolov Date: Tue, 2 Mar 2021 21:33:26 +0100 Subject: [PATCH 1/4] 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/4] 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/4] 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): From 444800759c69709635f189a1e537a15f9080aafc Mon Sep 17 00:00:00 2001 From: Jiashuo Li Date: Fri, 26 Mar 2021 13:51:20 +0800 Subject: [PATCH 4/4] refine --- examples/exapp2 | 26 +++++++++++++++++++++++++ knack/parser.py | 46 +++++++++++--------------------------------- tests/test_parser.py | 23 ++++++++++++++++++++-- 3 files changed, 58 insertions(+), 37 deletions(-) diff --git a/examples/exapp2 b/examples/exapp2 index 7a27065..7135eac 100644 --- a/examples/exapp2 +++ b/examples/exapp2 @@ -59,6 +59,16 @@ type: group short-summary: An experimental command group """ +helps['demo'] = """ +type: group +short-summary: A command group for demos. +""" + +helps['demo arg'] = """ +type: group +short-summary: A command showing how to use arguments. +""" + def abc_show_command_handler(): """ @@ -151,6 +161,13 @@ def hello_command_handler(greetings=None): return ['Hello World!', greetings] +def demo_arg_handler(move=None): + if move: + print("Your move was: {}".format(move)) + return + print("Nothing to do.") + + WELCOME_MESSAGE = r""" _____ _ _____ / ____| | |_ _| @@ -179,6 +196,7 @@ class MyCommandsLoader(CLICommandsLoader): g.command('hello', 'hello_command_handler', confirmation=True) g.command('sample-json', 'sample_json_handler') g.command('sample-logger', 'sample_logger_handler') + with CommandGroup(self, 'abc', '__main__#{}') as g: g.command('list', 'abc_list_command_handler') g.command('show', 'abc_show_command_handler') @@ -202,12 +220,20 @@ class MyCommandsLoader(CLICommandsLoader): # A deprecated command group with CommandGroup(self, 'dep', '__main__#{}', deprecate_info=g.deprecate(redirect='ga', hide='1.0.0')) as g: g.command('range', 'range_command_handler') + + with CommandGroup(self, 'demo', '__main__#{}') as g: + g.command('arg', 'demo_arg_handler') + return super(MyCommandsLoader, self).load_command_table(args) def load_arguments(self, command): with ArgumentsContext(self, 'ga range') as ac: ac.argument('start', type=int, is_preview=True) ac.argument('end', type=int, is_experimental=True) + + with ArgumentsContext(self, 'demo arg') as ac: + ac.argument('move', choices=['rock', 'paper', 'scissors']) + super(MyCommandsLoader, self).load_arguments(command) diff --git a/knack/parser.py b/knack/parser.py index e79b744..1e7782f 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -30,20 +30,6 @@ ] -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 @@ -280,34 +266,24 @@ def _check_value(self, action, value): import sys if action.choices is not None and value not in action.choices: - 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, - 'action': _get_action_name(action), - 'choices': ', '.join(map(repr, candidates)) - } - - if is_command: - # parser has no `command_source`, value is part of command itself + if action.dest in ["_command", "_subcommand"]: + # Command error_msg = "{prog}: '{value}' is not in the '{prog}' command group. See '{prog} --help'.".format( prog=self.prog, value=value) logger.error(error_msg) + # Show suggestions + candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) if candidates: - suggestion_msg = "\nThe most similar choice{s} to '{value}' {verb}:\n".format(**print_args) + suggestion_msg = "\nThe most similar choices to '{value}':\n".format(value=value) 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) + # Argument + error_msg = "{prog}: '{value}' is not a valid value for '{name}'.".format( + prog=self.prog, value=value, + name=argparse._get_action_name(action)) # pylint: disable=protected-access 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]) + # Show all allowed values + suggestion_msg = "Allowed values: " + ', '.join(action.choices) print(suggestion_msg, file=sys.stderr) self.exit(2) diff --git a/tests/test_parser.py b/tests/test_parser.py index 977b51b..a491b97 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -9,7 +9,7 @@ from knack.parser import CLICommandParser from knack.commands import CLICommand from knack.arguments import enum_choice_list -from tests.util import MockContext +from tests.util import MockContext, redirect_io class TestParser(unittest.TestCase): @@ -83,7 +83,7 @@ def test_handler(): parser.parse_args('test command -req yep'.split()) self.assertTrue(CLICommandParser.error.called) - def test_case_insensitive_enum_choices(self): + def _enum_parser(self): from enum import Enum class TestEnum(Enum): # pylint: disable=too-few-public-methods @@ -102,7 +102,10 @@ def test_handler(): parser = CLICommandParser() parser.load_command_table(self.mock_ctx.commands_loader) + return parser + def test_case_insensitive_enum_choices(self): + parser = self._enum_parser() args = parser.parse_args('test command --opt alL_cAps'.split()) self.assertEqual(args.opt, 'ALL_CAPS') @@ -112,6 +115,22 @@ def test_handler(): args = parser.parse_args('test command --opt sNake_CASE'.split()) self.assertEqual(args.opt, 'snake_case') + @redirect_io + def test_check_value_invalid_command(self): + parser = self._enum_parser() + with self.assertRaises(SystemExit) as cm: + parser.parse_args('test command1'.split()) # 'command1' is invalid + actual = self.io.getvalue() + assert "is not in the" in actual and "command group" in actual + + @redirect_io + def test_check_value_invalid_argument_value(self): + parser = self._enum_parser() + with self.assertRaises(SystemExit) as cm: + parser.parse_args('test command --opt foo'.split()) # 'foo' is invalid + actual = self.io.getvalue() + assert "is not a valid value for" in actual + def test_cli_ctx_type_error(self): with self.assertRaises(TypeError): CLICommandParser(cli_ctx=object())