Repository: python-cmd2/cmd2

Issue:
Handle mutually exclusive group in AutoCompleter
AutoCompleter should be improved to keep track of what arg was used in a mutually exclusive group and not offer to tab complete the other args in the group.

While these groups typically just include flags, it is possible to a have a positional as in the following case.

```Python
parser = Cmd2ArgumentParser()

group = parser.add_mutually_exclusive_group(required=True)
group.add_argument('optional_pos', nargs=argparse.OPTIONAL) 
group.add_argument('--flag')

parser.add_argument('required_pos')
```

If `--flag` had been used, then AutoCompleter would need to skip over `optional_pos` to `required_pos`.



Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8135a086..5c4383a8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,7 +1,13 @@
 ## 0.9.18 (TBD, 2019)
 * Bug Fixes
     * Fixed bug introduced in 0.9.17 where help functions for hidden and disabled commands were not being filtered
-     out as help topics
+    out as help topics
+* Enhancements
+    * `AutoCompleter` now handles argparse's mutually exclusive groups. It will not tab complete flag names or positionals
+    for already completed groups. It also will print an error if you try tab completing a flag's value if the flag
+    belongs to a completed group.
+    * `AutoCompleter` now uses the passed-in parser's help formatter to generate hint text. This gives help and
+    hint text for an argument consistent formatting.
 
 ## 0.9.17 (September 23, 2019)
 * Bug Fixes
diff --git a/cmd2/argparse_completer.py b/cmd2/argparse_completer.py
index a866b3ff..3a66a45d 100644
--- a/cmd2/argparse_completer.py
+++ b/cmd2/argparse_completer.py
@@ -10,11 +10,12 @@ import argparse
 import inspect
 import numbers
 import shutil
+from collections import deque
 from typing import Dict, List, Optional, Union
 
 from . import cmd2
 from . import utils
-from .ansi import ansi_safe_wcswidth, style_error
+from .ansi import ansi_aware_write, ansi_safe_wcswidth, style_error
 from .argparse_custom import ATTR_CHOICES_CALLABLE, INFINITY, generate_range_error
 from .argparse_custom import ATTR_SUPPRESS_TAB_HINT, ATTR_DESCRIPTIVE_COMPLETION_HEADER, ATTR_NARGS_RANGE
 from .argparse_custom import ChoicesCallable, CompletionError, CompletionItem
@@ -116,13 +117,13 @@ class AutoCompleter(object):
             parent_tokens = dict()
         self._parent_tokens = parent_tokens
 
-        self._flags = []  # all flags in this command
-        self._flag_to_action = {}  # maps flags to the argparse action object
-        self._positional_actions = []  # actions for positional arguments (by position index)
-        self._subcommand_action = None  # this will be set if self._parser has subcommands
+        self._flags = []                      # all flags in this command
+        self._flag_to_action = {}             # maps flags to the argparse action object
+        self._positional_actions = []         # actions for positional arguments (by position index)
+        self._subcommand_action = None        # this will be set if self._parser has subcommands
 
         # Start digging through the argparse structures.
-        #  _actions is the top level container of parameter definitions
+        # _actions is the top level container of parameter definitions
         for action in self._parser._actions:
             # if the parameter is flag based, it will have option_strings
             if action.option_strings:
@@ -143,9 +144,8 @@ class AutoCompleter(object):
         if not tokens:
             return []
 
-        # Count which positional argument index we're at now. Loop through all tokens on the command line so far
-        # Skip any flags or flag parameter tokens
-        next_pos_arg_index = 0
+        # Positionals args that are left to parse
+        remaining_positionals = deque(self._positional_actions)
 
         # This gets set to True when flags will no longer be processed as argparse flags
         # That can happen when -- is used or an argument with nargs=argparse.REMAINDER is used
@@ -163,12 +163,58 @@ class AutoCompleter(object):
         # Keeps track of arguments we've seen and any tokens they consumed
         consumed_arg_values = dict()  # dict(arg_name -> List[tokens])
 
+        # Completed mutually exclusive groups
+        completed_mutex_groups = dict()  # dict(argparse._MutuallyExclusiveGroup -> Action which completed group)
+
         def consume_argument(arg_state: AutoCompleter._ArgumentState) -> None:
             """Consuming token as an argument"""
             arg_state.count += 1
             consumed_arg_values.setdefault(arg_state.action.dest, [])
             consumed_arg_values[arg_state.action.dest].append(token)
 
+        def update_mutex_groups(arg_action: argparse.Action) -> bool:
+            """
+            Check if an argument belongs to a mutually exclusive group and either mark that group
+            as complete or print an error if the group has already been completed
+            :param arg_action: the action of the argument
+            :return: False if the group has already been completed and there is a conflict, otherwise True
+            """
+            # Check if this action is in a mutually exclusive group
+            for group in self._parser._mutually_exclusive_groups:
+                if arg_action in group._group_actions:
+
+                    # Check if the group this action belongs to has already been completed
+                    if group in completed_mutex_groups:
+
+                        # If this is the action that completed the group, then there is no error
+                        # since it's allowed to appear on the command line more than once.
+                        completer_action = completed_mutex_groups[group]
+                        if arg_action == completer_action:
+                            return True
+
+                        error = style_error("\nError: argument {}: not allowed with argument {}\n".
+                                            format(argparse._get_action_name(arg_action),
+                                                   argparse._get_action_name(completer_action)))
+                        self._print_message(error)
+                        return False
+
+                    # Mark that this action completed the group
+                    completed_mutex_groups[group] = arg_action
+
+                    # Don't tab complete any of the other args in the group
+                    for group_action in group._group_actions:
+                        if group_action == arg_action:
+                            continue
+                        elif group_action in self._flag_to_action.values():
+                            matched_flags.extend(group_action.option_strings)
+                        elif group_action in remaining_positionals:
+                            remaining_positionals.remove(group_action)
+
+                    # Arg can only be in one group, so we are done
+                    break
+
+            return True
+
         #############################################################################################
         # Parse all but the last token
         #############################################################################################
@@ -222,6 +268,9 @@ class AutoCompleter(object):
                         action = self._flag_to_action[candidates_flags[0]]
 
                 if action is not None:
+                    if not update_mutex_groups(action):
+                        return []
+
                     if isinstance(action, (argparse._AppendAction,
                                            argparse._AppendConstAction,
                                            argparse._CountAction)):
@@ -229,7 +278,7 @@ class AutoCompleter(object):
                         # Therefore don't erase any tokens already consumed for this flag
                         consumed_arg_values.setdefault(action.dest, [])
                     else:
-                        # This flag is not resusable, so mark that we've seen it
+                        # This flag is not reusable, so mark that we've seen it
                         matched_flags.extend(action.option_strings)
 
                         # It's possible we already have consumed values for this flag if it was used
@@ -255,12 +304,9 @@ class AutoCompleter(object):
             else:
                 # If we aren't current tracking a positional, then get the next positional arg to handle this token
                 if pos_arg_state is None:
-                    pos_index = next_pos_arg_index
-                    next_pos_arg_index += 1
-
-                    # Make sure we are still have positional arguments to fill
-                    if pos_index < len(self._positional_actions):
-                        action = self._positional_actions[pos_index]
+                    # Make sure we are still have positional arguments to parse
+                    if remaining_positionals:
+                        action = remaining_positionals.popleft()
 
                         # Are we at a subcommand? If so, forward to the matching completer
                         if action == self._subcommand_action:
@@ -285,6 +331,10 @@ class AutoCompleter(object):
 
                 # Check if we have a positional to consume this token
                 if pos_arg_state is not None:
+                    # No need to check for an error since we remove a completed group's positional from
+                    # remaining_positionals which means this action can't belong to a completed mutex group
+                    update_mutex_groups(pos_arg_state.action)
+
                     consume_argument(pos_arg_state)
 
                     # No more flags are allowed if this is a REMAINDER argument
@@ -295,10 +345,9 @@ class AutoCompleter(object):
                     elif pos_arg_state.count >= pos_arg_state.max:
                         pos_arg_state = None
 
-                        # Check if this a case in which we've finished all positionals before one that has nargs
-                        # set to argparse.REMAINDER. At this point argparse allows no more flags to be processed.
-                        if next_pos_arg_index < len(self._positional_actions) and \
-                                self._positional_actions[next_pos_arg_index].nargs == argparse.REMAINDER:
+                        # Check if the next positional has nargs set to argparse.REMAINDER.
+                        # At this point argparse allows no more flags to be processed.
+                        if remaining_positionals and remaining_positionals[0].nargs == argparse.REMAINDER:
                             skip_remaining_flags = True
 
         #############################################################################################
@@ -338,12 +387,11 @@ class AutoCompleter(object):
                 return []
 
         # Otherwise check if we have a positional to complete
-        elif pos_arg_state is not None or next_pos_arg_index < len(self._positional_actions):
+        elif pos_arg_state is not None or remaining_positionals:
 
             # If we aren't current tracking a positional, then get the next positional arg to handle this token
             if pos_arg_state is None:
-                pos_index = next_pos_arg_index
-                action = self._positional_actions[pos_index]
+                action = remaining_positionals.popleft()
                 pos_arg_state = AutoCompleter._ArgumentState(action)
 
             try:
@@ -532,23 +580,11 @@ class AutoCompleter(object):
 
         return self._format_completions(arg_action, results)
 
-    @staticmethod
-    def _format_message_prefix(arg_action: argparse.Action) -> str:
-        """Format the arg prefix text that appears before messages printed to the user"""
-        # Check if this is a flag
-        if arg_action.option_strings:
-            flags = ', '.join(arg_action.option_strings)
-            param = ' ' + str(arg_action.dest).upper()
-            return '{}{}'.format(flags, param)
-
-        # Otherwise this is a positional
-        else:
-            return '{}'.format(str(arg_action.dest).upper())
-
     @staticmethod
     def _print_message(msg: str) -> None:
         """Print a message instead of tab completions and redraw the prompt and input line"""
-        print(msg)
+        import sys
+        ansi_aware_write(sys.stdout, msg + '\n')
         rl_force_redisplay()
 
     def _print_arg_hint(self, arg_action: argparse.Action) -> None:
@@ -558,36 +594,27 @@ class AutoCompleter(object):
         """
         # Check if hinting is disabled
         suppress_hint = getattr(arg_action, ATTR_SUPPRESS_TAB_HINT, False)
-        if suppress_hint or arg_action.help == argparse.SUPPRESS or arg_action.dest == argparse.SUPPRESS:
+        if suppress_hint or arg_action.help == argparse.SUPPRESS:
             return
 
-        prefix = self._format_message_prefix(arg_action)
-        prefix = '  {0: <{width}}    '.format(prefix, width=20)
-        pref_len = len(prefix)
-
-        help_text = '' if arg_action.help is None else arg_action.help
-        help_lines = help_text.splitlines()
-
-        if len(help_lines) == 1:
-            self._print_message('\nHint:\n{}{}\n'.format(prefix, help_lines[0]))
-        else:
-            out_str = '\n{}'.format(prefix)
-            out_str += '\n{0: <{width}}'.format('', width=pref_len).join(help_lines)
-            self._print_message('\nHint:' + out_str + '\n')
+        # Use the parser's help formatter to print just this action's help text
+        formatter = self._parser._get_formatter()
+        formatter.start_section("Hint")
+        formatter.add_argument(arg_action)
+        formatter.end_section()
+        out_str = formatter.format_help()
+        self._print_message('\n' + out_str)
 
     def _print_unfinished_flag_error(self, flag_arg_state: _ArgumentState) -> None:
         """
         Print an error during tab completion when the user has not finished the current flag
         :param flag_arg_state: information about the unfinished flag action
         """
-        prefix = self._format_message_prefix(flag_arg_state.action)
-
-        out_str = "\nError:\n"
-        out_str += '  {0: <{width}}    '.format(prefix, width=20)
-        out_str += generate_range_error(flag_arg_state.min, flag_arg_state.max)
-
-        out_str += ' ({} entered)'.format(flag_arg_state.count)
-        self._print_message(style_error('{}\n'.format(out_str)))
+        error = "\nError: argument {}: {} ({} entered)\n".\
+            format(argparse._get_action_name(flag_arg_state.action),
+                   generate_range_error(flag_arg_state.min, flag_arg_state.max),
+                   flag_arg_state.count)
+        self._print_message(style_error('{}'.format(error)))
 
     def _print_completion_error(self, arg_action: argparse.Action, completion_error: CompletionError) -> None:
         """
@@ -595,10 +622,6 @@ class AutoCompleter(object):
         :param arg_action: action being tab completed
         :param completion_error: error that occurred
         """
-        prefix = self._format_message_prefix(arg_action)
-
-        out_str = "\nError:\n"
-        out_str += '  {0: <{width}}    '.format(prefix, width=20)
-        out_str += str(completion_error)
-
-        self._print_message(style_error('{}\n'.format(out_str)))
+        error = ("\nError tab completing {}:\n"
+                 "  {}\n".format(argparse._get_action_name(arg_action), str(completion_error)))
+        self._print_message(style_error('{}'.format(error)))
diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py
old mode 100755
new mode 100644
index 50d562b4..5e1f9a72
--- a/cmd2/cmd2.py
+++ b/cmd2/cmd2.py
@@ -1848,7 +1848,7 @@ class Cmd(cmd.Cmd):
         """Keep accepting lines of input until the command is complete.
 
         There is some pretty hacky code here to handle some quirks of
-        self.pseudo_raw_input(). It returns a literal 'eof' if the input
+        self._pseudo_raw_input(). It returns a literal 'eof' if the input
         pipe runs out. We can't refactor it because we need to retain
         backwards compatibility with the standard library version of cmd.
 


Test patch:
diff --git a/tests/test_argparse_completer.py b/tests/test_argparse_completer.py
index 308a4d95..e54e49c2 100644
--- a/tests/test_argparse_completer.py
+++ b/tests/test_argparse_completer.py
@@ -67,12 +67,12 @@ class AutoCompleteTester(cmd2.Cmd):
 
     # Add subcommands to music
     music_subparsers = music_parser.add_subparsers()
-    music_create_parser = music_subparsers.add_parser('create', help='Create music')
+    music_create_parser = music_subparsers.add_parser('create', help='create music')
 
     # Add subcommands to music -> create
     music_create_subparsers = music_create_parser.add_subparsers()
-    music_create_jazz_parser = music_create_subparsers.add_parser('jazz', help='Create jazz')
-    music_create_rock_parser = music_create_subparsers.add_parser('rock', help='Create rocks')
+    music_create_jazz_parser = music_create_subparsers.add_parser('jazz', help='create jazz')
+    music_create_rock_parser = music_create_subparsers.add_parser('rock', help='create rocks')
 
     @with_argparser(music_parser)
     def do_music(self, args: argparse.Namespace) -> None:
@@ -84,10 +84,10 @@ class AutoCompleteTester(cmd2.Cmd):
 
     # Uses default flag prefix value (-)
     flag_parser = Cmd2ArgumentParser()
-    flag_parser.add_argument('-n', '--normal_flag', help='A normal flag', action='store_true')
-    flag_parser.add_argument('-a', '--append_flag', help='Append flag', action='append')
-    flag_parser.add_argument('-o', '--append_const_flag', help='Append const flag', action='append_const', const=True)
-    flag_parser.add_argument('-c', '--count_flag', help='Count flag', action='count')
+    flag_parser.add_argument('-n', '--normal_flag', help='a normal flag', action='store_true')
+    flag_parser.add_argument('-a', '--append_flag', help='append flag', action='append')
+    flag_parser.add_argument('-o', '--append_const_flag', help='append const flag', action='append_const', const=True)
+    flag_parser.add_argument('-c', '--count_flag', help='count flag', action='count')
     flag_parser.add_argument('-s', '--suppressed_flag', help=argparse.SUPPRESS, action='store_true')
     flag_parser.add_argument('-r', '--remainder_flag', nargs=argparse.REMAINDER, help='a remainder flag')
 
@@ -97,7 +97,7 @@ class AutoCompleteTester(cmd2.Cmd):
 
     # Uses non-default flag prefix value (+)
     plus_flag_parser = Cmd2ArgumentParser(prefix_chars='+')
-    plus_flag_parser.add_argument('+n', '++normal_flag', help='A normal flag', action='store_true')
+    plus_flag_parser.add_argument('+n', '++normal_flag', help='a normal flag', action='store_true')
 
     @with_argparser(plus_flag_parser)
     def do_plus_flag(self, args: argparse.Namespace) -> None:
@@ -251,6 +251,22 @@ class AutoCompleteTester(cmd2.Cmd):
     def do_arg_tokens(self, args: argparse.Namespace) -> None:
         pass
 
+    ############################################################################################################
+    # Begin code related to mutually exclusive groups
+    ############################################################################################################
+    mutex_parser = Cmd2ArgumentParser()
+
+    mutex_group = mutex_parser.add_mutually_exclusive_group(required=True)
+    mutex_group.add_argument('optional_pos', help='the optional positional', nargs=argparse.OPTIONAL)
+    mutex_group.add_argument('-f', '--flag', help='the flag arg')
+    mutex_group.add_argument('-o', '--other_flag', help='the other flag arg')
+
+    mutex_parser.add_argument('last_arg', help='the last arg')
+
+    @with_argparser(mutex_parser)
+    def do_mutex(self, args: argparse.Namespace) -> None:
+        pass
+
 
 @pytest.fixture
 def ac_app():
@@ -271,8 +287,16 @@ def test_help(ac_app, command):
     assert out1 == out2
 
 
+def test_bad_subcommand_help(ac_app):
+    # These should give the same output because the second one isn't using a
+    # real subcommand, so help will be called on the music command instead.
+    out1, err1 = run_cmd(ac_app, 'help music')
+    out2, err2 = run_cmd(ac_app, 'help music fake')
+    assert out1 == out2
+
+
 @pytest.mark.parametrize('command, text, completions', [
-    ('', 'mu', ['music ']),
+    ('', 'mus', ['music ']),
     ('music', 'cre', ['create ']),
     ('music', 'creab', []),
     ('music create', '', ['jazz', 'rock']),
@@ -654,7 +678,7 @@ def test_unfinished_flag_error(ac_app, command_and_args, text, is_error, capsys)
     complete_tester(text, line, begidx, endidx, ac_app)
 
     out, err = capsys.readouterr()
-    assert is_error == all(x in out for x in ["Error:\n", "expected"])
+    assert is_error == all(x in out for x in ["Error: argument", "expected"])
 
 
 def test_completion_items_default_header(ac_app):
@@ -709,24 +733,6 @@ def test_autocomp_hint(ac_app, command_and_args, text, has_hint, capsys):
     assert has_hint == ("Hint:\n" in out)
 
 
-def test_autocomp_hint_multiple_lines(ac_app, capsys):
-    text = ''
-    line = 'hint {}'.format(text)
-    endidx = len(line)
-    begidx = endidx - len(text)
-
-    first_match = complete_tester(text, line, begidx, endidx, ac_app)
-    out, err = capsys.readouterr()
-
-    assert first_match is None
-    assert out == '''
-Hint:
-  HINT_POS                here is a hint
-                          with new lines
-
-'''
-
-
 def test_autocomp_hint_no_help_text(ac_app, capsys):
     text = ''
     line = 'hint foo {}'.format(text)
@@ -788,6 +794,45 @@ def test_arg_tokens(ac_app, command_and_args, completions):
     assert ac_app.completion_matches == sorted(completions, key=ac_app.default_sort_key)
 
 
+@pytest.mark.parametrize('command_and_args, text, output_contains, first_match', [
+    # Group isn't done. Hint will show for optional positional and no completions returned
+    ('mutex', '', 'the optional positional', None),
+
+    # Group isn't done. Flag name will still complete.
+    ('mutex', '--fl', '', '--flag '),
+
+    # Group isn't done. Flag hint will show.
+    ('mutex --flag', '', 'the flag arg', None),
+
+    # Group finished by optional positional. No flag name will complete.
+    ('mutex pos_val', '--fl', '', None),
+
+    # Group finished by optional positional. Error will display trying to complete the flag's value.
+    ('mutex pos_val --flag', '', 'f/--flag: not allowed with argument optional_pos', None),
+
+    # Group finished by --flag. Optional positional will be skipped and last_arg will show its hint.
+    ('mutex --flag flag_val', '', 'the last arg', None),
+
+    # Group finished by --flag. Other flag name won't complete.
+    ('mutex --flag flag_val', '--oth', '', None),
+
+    # Group finished by --flag. Error will display trying to complete other flag's value.
+    ('mutex --flag flag_val --other', '', '-o/--other_flag: not allowed with argument -f/--flag', None),
+
+    # Group finished by --flag. That same flag can be used again so it's hint will show.
+    ('mutex --flag flag_val --flag', '', 'the flag arg', None)
+])
+def test_complete_mutex_group(ac_app, command_and_args, text, output_contains, first_match, capsys):
+    line = '{} {}'.format(command_and_args, text)
+    endidx = len(line)
+    begidx = endidx - len(text)
+
+    assert first_match == complete_tester(text, line, begidx, endidx, ac_app)
+
+    out, err = capsys.readouterr()
+    assert output_contains in out
+
+
 def test_single_prefix_char():
     from cmd2.argparse_completer import _single_prefix_char
     parser = Cmd2ArgumentParser(prefix_chars='-+')