Clean up history command

3 files changed, 152 insertions, 110 deletions

diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py
index b3a61212..8dafffaf 100644
--- a/cmd2/cmd2.py
+++ b/cmd2/cmd2.py
@@ -3213,15 +3213,22 @@ class Cmd(cmd.Cmd):
             # 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
+            except ValueError:
+                pass
+
             if '..' in arg or ':' in arg:
-                try:
-                    # Get a slice of history
-                    history = self.history.span(arg)
-                except IndexError:
-                    history = self.history.get(arg)
+                # Get a slice of history
+                history = self.history.span(arg)
+            elif arg_is_int:
+                history = [self.history.get(arg)]
+            elif arg.startswith(r'/') and arg.endswith(r'/'):
+                history = self.history.regex_search(arg)
             else:
-                # Get item(s) from history by index or string search
-                history = self.history.get(arg)
+                history = self.history.str_search(arg)
         else:
             # If no arg given, then retrieve the entire history
             cowardly_refuse_to_run = True
diff --git a/cmd2/history.py b/cmd2/history.py
index 77b1da51..819989b1 100644
--- a/cmd2/history.py
+++ b/cmd2/history.py
@@ -5,7 +5,7 @@ History management classes
 
 import re
 
-from typing import List, Optional, Union
+from typing import List, Union
 
 from . import utils
 from .parsing import Statement
@@ -73,26 +73,62 @@ class History(list):
     """
 
     # noinspection PyMethodMayBeStatic
-    def _zero_based_index(self, onebased: int) -> int:
+    def _zero_based_index(self, onebased: Union[int, str]) -> int:
         """Convert a one-based index to a zero-based index."""
-        result = onebased
+        result = int(onebased)
         if result > 0:
             result -= 1
         return result
 
-    def _to_index(self, raw: str) -> Optional[int]:
-        if raw:
-            result = self._zero_based_index(int(raw))
-        else:
-            result = None
-        return result
+    def append(self, new: Statement) -> None:
+        """Append a HistoryItem to end of the History list
 
-    spanpattern = re.compile(r'^\s*(?P<start>-?\d+)?\s*(?P<separator>:|(\.{2,}))?\s*(?P<end>-?\d+)?\s*$')
+        :param new: command line to convert to HistoryItem and add to the end of the History list
+        """
+        new = HistoryItem(new)
+        list.append(self, new)
+        new.idx = len(self)
 
-    def span(self, raw: str) -> List[HistoryItem]:
-        """Parses the input string and return a slice from the History list.
+    def get(self, index: Union[int, str]) -> HistoryItem:
+        """Get item from the History list using 1-based indexing.
 
-        :param raw: string potentially containing a span
+        :param index: optional item to get (index as either integer or string)
+        :return: a single HistoryItem
+        """
+        index = int(index)
+        if index == 0:
+            raise IndexError
+        elif index < 0:
+            return self[index]
+        else:
+            return self[index - 1]
+
+    # This regular expression parses input for the span() method. There are five parts:
+    #
+    #    ^\s*                          matches any whitespace at the beginning of the
+    #                                  input. This is here so you don't have to trim the input
+    #
+    #    (?P<start>-?\d+)?             create a capture group named 'start' which matches one
+    #                                  or more digits, optionally preceeded by a minus sign. This
+    #                                  group is optional so that we can match a string like '..2'
+    #
+    #    (?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'
+    #
+    #    (?P<end>-?\d+)?               create a capture group named 'end' which matches one or more
+    #                                  digits, optionally preceeded by a minus sign. This group is
+    #                                  optional so that we can match a string like ':' or '5:'
+    #
+    #    \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>-?\d+)?(?P<separator>:|(\.{2,}))?(?P<end>-?\d+)?\s*$')
+
+    def span(self, span: str) -> List[HistoryItem]:
+        """Return an index or slice of the History list,
+
+        :param raw: string containing an index or a slice
         :return: a list of HistoryItems
 
         This method can accommodate input in any of these forms:
@@ -107,84 +143,71 @@ class History(list):
 
         Different from native python indexing and slicing of arrays, this method
         uses 1-based array numbering. Users who are not programmers can't grok
-        0 based numbering. Programmers can grok either. Which reminds me, there
-        are only two hard problems in programming:
+        0 based numbering. Programmers can usually grok either. Which reminds me,
+        there are only two hard problems in programming:
 
         - naming
         - cache invalidation
         - off by one errors
 
         """
-        if raw.lower() in ('*', '-', 'all'):
-            raw = ':'
-        results = self.spanpattern.search(raw)
+        if span.lower() in ('*', '-', 'all'):
+            span = ':'
+        results = self.spanpattern.search(span)
         if not results:
-            raise IndexError
-        if not results.group('separator'):
-            return [self[self._to_index(results.group('start'))]]
-        start = self._to_index(results.group('start')) or 0  # Ensure start is not None
-        end = self._to_index(results.group('end'))
-        reverse = False
-        if end is not None:
-            if end < start:
-                (start, end) = (end, start)
-                reverse = True
-            end += 1
-        result = self[start:end]
-        if reverse:
-            result.reverse()
+            # our regex doesn't match the input, bail out
+            raise ValueError
+
+        sep = results.group('separator')
+        start = results.group('start')
+        if start:
+            start = self._zero_based_index(start)
+        end = results.group('end')
+        if end:
+            end = int(end)
+
+        if start is not None and end is not None:
+            # we have both start and end, return a slice of history, unless both are negative
+            if start < 0 and end < 0:
+                raise ValueError
+            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:
+            result = self[:end]
+        elif start is not None:
+            # there was no separator so it's either a posative or negative integer
+            result = [self[start]]
+        else:
+            # we just have a separator, return the whole list
+            result = self[:]
         return result
 
-    rangePattern = re.compile(r'^\s*(?P<start>[\d]+)?\s*-\s*(?P<end>[\d]+)?\s*$')
-
-    def append(self, new: Statement) -> None:
-        """Append a HistoryItem to end of the History list
-
-        :param new: command line to convert to HistoryItem and add to the end of the History list
-        """
-        new = HistoryItem(new)
-        list.append(self, new)
-        new.idx = len(self)
-
-    def get(self, index: Union[int, str]) -> HistoryItem:
-        """Get item from the History list using 1-based indexing.
+    def str_search(self, search: str) -> List[HistoryItem]:
+        """Find history items which contain a given string
 
-        :param index: optional item to get (index as either integer or string)
-        :return: a single HistoryItem
+        :param search: the string to search for
+        :return: a list of history items, or an empty list if the string was not found
         """
-        index = int(index)
-        if index == 0:
-            raise IndexError
-        elif index < 0:
-            return self[index]
-        else:
-            return self[index - 1]
-
-
-
-    def str_search(self, search: str) -> List[HistoryItem]:
-        pass
+        def isin(history_item):
+            """filter function for string search of history"""
+            sloppy = utils.norm_fold(search)
+            return sloppy in utils.norm_fold(history_item) or sloppy in utils.norm_fold(history_item.expanded)
+        return [item for item in self if isin(item)]
 
     def regex_search(self, regex: str) -> List[HistoryItem]:
-            regex = regex.strip()
-
-            if regex.startswith(r'/') and regex.endswith(r'/'):
-                finder = re.compile(regex[1:-1], re.DOTALL | re.MULTILINE | re.IGNORECASE)
+        """Find history items which match a given regular expression
 
-                def isin(hi):
-                    """Listcomp filter function for doing a regular expression search of History.
-
-                    :param hi: HistoryItem
-                    :return: bool - True if search matches
-                    """
-                    return finder.search(hi) or finder.search(hi.expanded)
-            else:
-                def isin(hi):
-                    """Listcomp filter function for doing a case-insensitive string search of History.
-
-                    :param hi: HistoryItem
-                    :return: bool - True if search matches
-                    """
-                    srch = utils.norm_fold(regex)
-                    return srch in utils.norm_fold(hi) or srch in utils.norm_fold(hi.expanded)
-            return [itm for itm in self if isin(itm)]
+        :param regex: the regular expression to search for.
+        :return: a list of history items, or an empty list if the string was not found
+        """
+        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):
+            """filter function for doing a regular expression search of history"""
+            return finder.search(hi) or finder.search(hi.expanded)
+        return [itm for itm in self if isin(itm)]
diff --git a/tests/test_history.py b/tests/test_history.py
index e354a711..1554df5e 100644
--- a/tests/test_history.py
+++ b/tests/test_history.py
@@ -63,16 +63,28 @@ def hist():
     return h
 
 def test_history_class_span(hist):
-    h = hist
-    assert h == ['first', 'second', 'third', 'fourth']
-    assert h.span('-2..') == ['third', 'fourth']
-    assert h.span('2..3') == ['second', 'third']    # Inclusive of end
-    assert h.span('3') == ['third']
-    assert h.span(':') == h
-    assert h.span('2..') == ['second', 'third', 'fourth']
-    assert h.span('-1') == ['fourth']
-    assert h.span('-2..-3') == ['third', 'second']
-    assert h.span('*') == h
+    for tryit in ['*', ':', '-', 'all', 'ALL']:
+        assert hist.span(tryit) == hist
+
+    assert hist.span('3') == ['third']
+    assert hist.span('-1') == ['fourth']
+
+    assert hist.span('2..') == ['second', 'third', 'fourth']
+    assert hist.span('2:') == ['second', 'third', 'fourth']
+
+    assert hist.span('-2..') == ['third', 'fourth']
+    assert hist.span('-2:') == ['third', 'fourth']
+
+    assert hist.span('1..3') == ['first', 'second', 'third']
+    assert hist.span('1:3') == ['first', 'second', 'third']
+
+    assert hist.span(':-2') == ['first', 'second']
+    assert hist.span('..-2') == ['first', 'second']
+
+    value_errors = ['fred', 'fred:joe', 'a..b', '2 ..', '1 : 3', '-2..-3' ]
+    for tryit in value_errors:
+        with pytest.raises(ValueError):
+            hist.span(tryit)
 
 def test_history_class_get(hist):
     assert hist.get('1') == 'first'
@@ -101,10 +113,12 @@ def test_history_class_get(hist):
         hist.get(None)
 
 def test_history_str_search(hist):
-    assert hist.get('ir') == ['first', 'third']
+    assert hist.str_search('ir') == ['first', 'third']
+    assert hist.str_search('rth') == ['fourth']
 
 def test_history_regex_search(hist):
-    assert hist.get('/i.*d/') == ['third']
+    assert hist.regex_search('/i.*d/') == ['third']
+    assert hist.regex_search('s[a-z]+ond') == ['second']
 
 def test_base_history(base_app):
     run_cmd(base_app, 'help')
@@ -222,11 +236,9 @@ def test_history_with_span_index_error(base_app):
     run_cmd(base_app, 'help')
     run_cmd(base_app, 'help history')
     run_cmd(base_app, '!ls -hal :')
-    out = run_cmd(base_app, 'history "hal :"')
-    expected = normalize("""
-    3  !ls -hal :
-""")
-    assert out == expected
+    with pytest.raises(ValueError):
+        _, err = run_cmd(base_app, 'history "hal :"')
+        assert "ValueError" in err
 
 def test_history_output_file(base_app):
     run_cmd(base_app, 'help')
@@ -380,8 +392,8 @@ def test_existing_history_file(hist_file, capsys):
         pass
 
     # Create a new cmd2 app
-    app = cmd2.Cmd(persistent_history_file=hist_file)
-    out, err = capsys.readouterr()
+    cmd2.Cmd(persistent_history_file=hist_file)
+    _, err = capsys.readouterr()
 
     # Make sure there were no errors
     assert err == ''
@@ -403,8 +415,8 @@ def test_new_history_file(hist_file, capsys):
         pass
 
     # Create a new cmd2 app
-    app = cmd2.Cmd(persistent_history_file=hist_file)
-    out, err = capsys.readouterr()
+    cmd2.Cmd(persistent_history_file=hist_file)
+    _, err = capsys.readouterr()
 
     # Make sure there were no errors
     assert err == ''
@@ -420,8 +432,8 @@ def test_bad_history_file_path(capsys, request):
     test_dir = os.path.dirname(request.module.__file__)
 
     # Create a new cmd2 app
-    app = cmd2.Cmd(persistent_history_file=test_dir)
-    out, err = capsys.readouterr()
+    cmd2.Cmd(persistent_history_file=test_dir)
+    _, err = capsys.readouterr()
 
     if sys.platform == 'win32':
         # pyreadline masks the read exception. Therefore the bad path error occurs when trying to write the file.