Add error message for invalid argument value (#244)

Co-authored-by: Vadim Frolov <[email protected]>

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)
 
 

knack/parser.py

@@ -266,19 +266,24 @@ def _check_value(self, action, value):
         import sys
 
         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)
-            if candidates:
-                print_args = {
-                    's': 's' if len(candidates) > 1 else '',
-                    'verb': 'are' if len(candidates) > 1 else 'is',
-                    'value': value
-                }
-                suggestion_msg = "\nThe most similar choice{s} to '{value}' {verb}:\n".format(**print_args)
-                suggestion_msg += '\n'.join(['\t' + candidate for candidate in candidates])
+            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 choices to '{value}':\n".format(value=value)
+                    suggestion_msg += '\n'.join(['\t' + candidate for candidate in candidates])
+                    print(suggestion_msg, file=sys.stderr)
+            else:
+                # 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)
+                # Show all allowed values
+                suggestion_msg = "Allowed values: " + ', '.join(action.choices)
                 print(suggestion_msg, file=sys.stderr)
-
             self.exit(2)

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())