Merge pull request #1065 from python-cmd2/history_fix

History fix

Author: kmvanbrunt

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## 2.0.0 (TBD, 2021)
 * Bug Fixes
+  * Fixed issue where history indexes could get repeated
   * Fixed issue where TableCreator was tossing blank last lines
 * Breaking Changes
     * `cmd2` 2.0 supports Python 3.6+ (removed support for Python 3.5)

cmd2/cmd2.py

@@ -788,7 +788,7 @@ def find_subcommand(action: argparse.ArgumentParser, subcmd_names: List[str]) ->
                             if choice_name == cur_subcmd:
                                 return find_subcommand(choice, subcmd_names)
                         break
-                raise CommandSetRegistrationError('Could not find sub-command "{}"'.format(full_command_name))
+                raise CommandSetRegistrationError('Could not find subcommand "{}"'.format(full_command_name))
 
             target_parser = find_subcommand(command_parser, subcommand_names)
 
@@ -4235,13 +4235,13 @@ def do_history(self, args: argparse.Namespace) -> Optional[bool]:
                 self.perror("Cowardly refusing to run all previously entered commands.")
                 self.perror("If this is what you want to do, specify '1:' as the range of history.")
             else:
-                return self.runcmds_plus_hooks(history)
+                return self.runcmds_plus_hooks(list(history.values()))
         elif args.edit:
             import tempfile
 
             fd, fname = tempfile.mkstemp(suffix='.txt', text=True)
             with os.fdopen(fd, 'w') as fobj:
-                for command in history:
+                for command in history.values():
                     if command.statement.multiline_command:
                         fobj.write('{}\n'.format(command.expanded))
                     else:
@@ -4255,7 +4255,7 @@ def do_history(self, args: argparse.Namespace) -> Optional[bool]:
         elif args.output_file:
             try:
                 with open(os.path.expanduser(args.output_file), 'w') as fobj:
-                    for item in history:
+                    for item in history.values():
                         if item.statement.multiline_command:
                             fobj.write('{}\n'.format(item.expanded))
                         else:
@@ -4266,33 +4266,31 @@ def do_history(self, args: argparse.Namespace) -> Optional[bool]:
             else:
                 self.pfeedback('{} command{} saved to {}'.format(len(history), plural, args.output_file))
         elif args.transcript:
-            self._generate_transcript(history, args.transcript)
+            self._generate_transcript(list(history.values()), args.transcript)
         else:
             # Display the history items retrieved
-            for hi in history:
-                self.poutput(hi.pr(script=args.script, expanded=args.expanded, verbose=args.verbose))
+            for idx, hi in history.items():
+                self.poutput(hi.pr(idx, script=args.script, expanded=args.expanded, verbose=args.verbose))
+
+    def _get_history(self, args: argparse.Namespace) -> Dict[int, HistoryItem]:
+        """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history.
 
-    def _get_history(self, args: argparse.Namespace) -> List[HistoryItem]:
-        """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history."""
+        This function returns a dictionary with history items keyed by their 1-based index in ascending order.
+        """
         if args.arg:
-            # If a character indicating a slice is present, retrieve a slice of the history
-            arg = args.arg
-            arg_is_int = False
             try:
-                int(arg)
-                arg_is_int = True
+                int_arg = int(args.arg)
+                return {int_arg: self.history.get(int_arg)}
             except ValueError:
                 pass
 
-            if '..' in arg or ':' in arg:
+            if '..' in args.arg or ':' in args.arg:
                 # Get a slice of history
-                history = self.history.span(arg, args.all)
-            elif arg_is_int:
-                history = [self.history.get(arg)]
-            elif arg.startswith(r'/') and arg.endswith(r'/'):
-                history = self.history.regex_search(arg, args.all)
+                history = self.history.span(args.arg, args.all)
+            elif args.arg.startswith(r'/') and args.arg.endswith(r'/'):
+                history = self.history.regex_search(args.arg, args.all)
             else:
-                history = self.history.str_search(arg, args.all)
+                history = self.history.str_search(args.arg, args.all)
         else:
             # Get a copy of the history so it doesn't get mutated while we are using it
             history = self.history.span(':', args.all)
@@ -5118,7 +5116,7 @@ def _resolve_func_self(self, cmd_support_func: Callable, cmd_self: Union[Command
         instance of each type, using type is a reasonably safe way to resolve the correct object instance
 
         :param cmd_support_func: command support function. This could be a completer or namespace provider
-        :param cmd_self: The `self` associated with the command or sub-command
+        :param cmd_self: The `self` associated with the command or subcommand
         :return:
         """
         # figure out what class the command support function was defined in

cmd2/history.py

@@ -5,7 +5,9 @@
 
 import re
 from typing import (
-    List,
+    Callable,
+    Dict,
+    Optional,
     Union,
 )
 
@@ -27,7 +29,6 @@ class HistoryItem:
     _ex_listformat = ' {:>4}x {}'
 
     statement = attr.ib(default=None, validator=attr.validators.instance_of(Statement))
-    idx = attr.ib(default=None, validator=attr.validators.instance_of(int))
 
     def __str__(self):
         """A convenient human readable representation of the history item"""
@@ -50,20 +51,24 @@ def expanded(self) -> str:
         """
         return self.statement.expanded_command_line
 
-    def pr(self, script=False, expanded=False, verbose=False) -> str:
+    def pr(self, idx: int, script: bool = False, expanded: bool = False, verbose: bool = False) -> str:
         """Represent this item in a pretty fashion suitable for printing.
 
         If you pass verbose=True, script and expanded will be ignored
 
+        :param idx: The 1-based index of this item in the history list
+        :param script: True if formatting for a script (No item numbers)
+        :param expanded: True if expanded command line should be printed
+        :param verbose: True if expanded and raw should both appear when they are different
         :return: pretty print string version of a HistoryItem
         """
         if verbose:
             raw = self.raw.rstrip()
             expanded = self.expanded
 
-            ret_str = self._listformat.format(self.idx, raw)
+            ret_str = self._listformat.format(idx, raw)
             if raw != expanded:
-                ret_str += '\n' + self._ex_listformat.format(self.idx, expanded)
+                ret_str += '\n' + self._ex_listformat.format(idx, expanded)
         else:
             if expanded:
                 ret_str = self.expanded
@@ -80,7 +85,7 @@ def pr(self, script=False, expanded=False, verbose=False) -> str:
 
             # Display a numbered list if not writing to a script
             if not script:
-                ret_str = self._listformat.format(self.idx, ret_str)
+                ret_str = self._listformat.format(idx, ret_str)
 
         return ret_str
 
@@ -121,21 +126,20 @@ def append(self, new: Statement) -> None:
         :param new: Statement object which will be composed into a HistoryItem
                     and added to the end of the list
         """
-        history_item = HistoryItem(new, len(self) + 1)
+        history_item = HistoryItem(new)
         super().append(history_item)
 
     def clear(self) -> None:
         """Remove all items from the History list."""
         super().clear()
         self.start_session()
 
-    def get(self, index: Union[int, str]) -> HistoryItem:
+    def get(self, index: int) -> HistoryItem:
         """Get item from the History list using 1-based indexing.
 
-        :param index: optional item to get (index as either integer or string)
+        :param index: optional item to get
         :return: a single :class:`~cmd2.history.HistoryItem`
         """
-        index = int(index)
         if index == 0:
             raise IndexError('The first command in history is command 1.')
         elif index < 0:
@@ -155,8 +159,7 @@ def get(self, index: Union[int, str]) -> HistoryItem:
     #                                  This regex will match 1, -1, 10, -10, but not 0 or -0.
     #
     #    (?P<separator>:|(\.{2,}))?    create a capture group named 'separator' which matches either
-    #                                  a colon or two periods. This group is optional so we can
-    #                                  match a string like '3'
+    #                                  a colon or two periods.
     #
     #    (?P<end>-?[1-9]{1}\d*)?       create a capture group named 'end' which matches an
     #                                  optional minus sign, followed by exactly one non-zero
@@ -168,19 +171,18 @@ def get(self, index: Union[int, str]) -> HistoryItem:
     #    \s*$                          match any whitespace at the end of the input. This is here so
     #                                  you don't have to trim the input
     #
-    spanpattern = re.compile(r'^\s*(?P<start>-?[1-9]\d*)?(?P<separator>:|(\.{2,}))?(?P<end>-?[1-9]\d*)?\s*$')
+    spanpattern = re.compile(r'^\s*(?P<start>-?[1-9]\d*)?(?P<separator>:|(\.{2,}))(?P<end>-?[1-9]\d*)?\s*$')
 
-    def span(self, span: str, include_persisted: bool = False) -> List[HistoryItem]:
-        """Return an index or slice of the History list,
+    def span(self, span: str, include_persisted: bool = False) -> Dict[int, HistoryItem]:
+        """Return a slice of the History list
 
         :param span: string containing an index or a slice
         :param include_persisted: if True, then retrieve full results including from persisted history
-        :return: a list of HistoryItems
+        :return: a dictionary of history items keyed by their 1-based index in ascending order,
+                 or an empty dictionary if no results were found
 
         This method can accommodate input in any of these forms:
 
-            a
-            -a
             a..b or a:b
             a.. or a:
             ..a or :a
@@ -197,89 +199,67 @@ def span(self, span: str, include_persisted: bool = False) -> List[HistoryItem]:
         - off by one errors
 
         """
-        if span.lower() in ('*', '-', 'all'):
-            span = ':'
         results = self.spanpattern.search(span)
         if not results:
             # our regex doesn't match the input, bail out
             raise ValueError('History indices must be positive or negative integers, and may not be zero.')
 
-        sep = results.group('separator')
         start = results.group('start')
         if start:
-            start = self._zero_based_index(start)
+            start = min(self._zero_based_index(start), len(self) - 1)
+            if start < 0:
+                start = max(0, len(self) + start)
+        else:
+            start = 0 if include_persisted else self.session_start_index
+
         end = results.group('end')
         if end:
-            end = int(end)
-            # modify end so it's inclusive of the last element
-            if end == -1:
-                # -1 as the end means include the last command in the array, which in pythonic
-                # terms means to not provide an ending index. If you put -1 as the ending index
-                # python excludes the last item in the list.
-                end = None
-            elif end < -1:
-                # if the ending is smaller than -1, make it one larger so it includes
-                # the element (python native indices exclude the last referenced element)
-                end += 1
-
-        if start is not None and end is not None:
-            # we have both start and end, return a slice of history
-            result = self[start:end]
-        elif start is not None and sep is not None:
-            # take a slice of the array
-            result = self[start:]
-        elif end is not None and sep is not None:
-            if include_persisted:
-                result = self[:end]
-            else:
-                result = self[self.session_start_index : end]
-        elif start is not None:
-            # there was no separator so it's either a positive or negative integer
-            result = [self[start]]
+            end = min(int(end), len(self))
+            if end < 0:
+                end = max(0, len(self) + end + 1)
         else:
-            # we just have a separator, return the whole list
-            if include_persisted:
-                result = self[:]
-            else:
-                result = self[self.session_start_index :]
-        return result
+            end = len(self)
 
-    def str_search(self, search: str, include_persisted: bool = False) -> List[HistoryItem]:
+        return self._build_result_dictionary(start, end)
+
+    def str_search(self, search: str, include_persisted: bool = False) -> Dict[int, HistoryItem]:
         """Find history items which contain a given string
 
         :param search: the string to search for
         :param include_persisted: if True, then search full history including persisted history
-        :return: a list of history items, or an empty list if the string was not found
+        :return: a dictionary of history items keyed by their 1-based index in ascending order,
+                 or an empty dictionary if the string was not found
         """
 
-        def isin(history_item):
+        def isin(history_item: HistoryItem) -> bool:
             """filter function for string search of history"""
             sloppy = utils.norm_fold(search)
             inraw = sloppy in utils.norm_fold(history_item.raw)
             inexpanded = sloppy in utils.norm_fold(history_item.expanded)
             return inraw or inexpanded
 
-        search_list = self if include_persisted else self[self.session_start_index :]
-        return [item for item in search_list if isin(item)]
+        start = 0 if include_persisted else self.session_start_index
+        return self._build_result_dictionary(start, len(self), isin)
 
-    def regex_search(self, regex: str, include_persisted: bool = False) -> List[HistoryItem]:
+    def regex_search(self, regex: str, include_persisted: bool = False) -> Dict[int, HistoryItem]:
         """Find history items which match a given regular expression
 
         :param regex: the regular expression to search for.
         :param include_persisted: if True, then search full history including persisted history
-        :return: a list of history items, or an empty list if the string was not found
+        :return: a dictionary of history items keyed by their 1-based index in ascending order,
+                 or an empty dictionary if the regex was not matched
         """
         regex = regex.strip()
         if regex.startswith(r'/') and regex.endswith(r'/'):
             regex = regex[1:-1]
         finder = re.compile(regex, re.DOTALL | re.MULTILINE)
 
-        def isin(hi):
+        def isin(hi: HistoryItem) -> bool:
             """filter function for doing a regular expression search of history"""
-            return finder.search(hi.raw) or finder.search(hi.expanded)
+            return bool(finder.search(hi.raw) or finder.search(hi.expanded))
 
-        search_list = self if include_persisted else self[self.session_start_index :]
-        return [itm for itm in search_list if isin(itm)]
+        start = 0 if include_persisted else self.session_start_index
+        return self._build_result_dictionary(start, len(self), isin)
 
     def truncate(self, max_length: int) -> None:
         """Truncate the length of the history, dropping the oldest items if necessary
@@ -294,3 +274,17 @@ def truncate(self, max_length: int) -> None:
         elif len(self) > max_length:
             last_element = len(self) - max_length
             del self[0:last_element]
+
+    def _build_result_dictionary(
+        self, start: int, end: int, filter_func: Optional[Callable[[HistoryItem], bool]] = None
+    ) -> Dict[int, HistoryItem]:
+        """
+        Build history search results
+        :param start: start index to search from
+        :param end: end index to stop searching (exclusive)
+        """
+        results: Dict[int, HistoryItem] = dict()
+        for index in range(start, end):
+            if filter_func is None or filter_func(self[index]):
+                results[index + 1] = self[index]
+        return results