diff options
author | Kevin Van Brunt <kmvanbrunt@gmail.com> | 2021-02-22 22:53:30 -0500 |
---|---|---|
committer | Kevin Van Brunt <kmvanbrunt@gmail.com> | 2021-02-23 10:06:53 -0500 |
commit | 71ed07c0578db8ae3f6d946db14e0d2630cac07e (patch) | |
tree | 17c8d6e192c44715ee38b18154cf82e74573166e | |
parent | 97f14b266e038a5d4acf7841d538cf37f066d328 (diff) | |
download | cmd2-git-71ed07c0578db8ae3f6d946db14e0d2630cac07e.tar.gz |
Fixed issue where HistoryItem indexes were being reused
-rw-r--r-- | cmd2/cmd2.py | 42 | ||||
-rw-r--r-- | cmd2/history.py | 124 | ||||
-rwxr-xr-x | tests/test_history.py | 181 |
3 files changed, 153 insertions, 194 deletions
diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 0b198111..5b30656f 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -787,7 +787,7 @@ class Cmd(cmd.Cmd): 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) @@ -4230,13 +4230,13 @@ class Cmd(cmd.Cmd): 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: @@ -4250,7 +4250,7 @@ class Cmd(cmd.Cmd): 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: @@ -4261,33 +4261,31 @@ class Cmd(cmd.Cmd): 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) @@ -5113,7 +5111,7 @@ class Cmd(cmd.Cmd): 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 diff --git a/cmd2/history.py b/cmd2/history.py index bc6c32ce..78377c06 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -5,7 +5,9 @@ History management classes 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 @@ class HistoryItem: """ 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 @@ class HistoryItem: # 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,7 +126,7 @@ class History(list): :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: @@ -129,13 +134,12 @@ class 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 @@ class History(list): # 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 @@ class History(list): # \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 @@ class History(list): - 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 @@ class History(list): 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 diff --git a/tests/test_history.py b/tests/test_history.py index 64ec4d28..41ae2a9e 100755 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -51,10 +51,10 @@ def hist(): h = History( [ - HistoryItem(Statement('', raw='first'), 1), - HistoryItem(Statement('', raw='second'), 2), - HistoryItem(Statement('', raw='third'), 3), - HistoryItem(Statement('', raw='fourth'), 4), + HistoryItem(Statement('', raw='first')), + HistoryItem(Statement('', raw='second')), + HistoryItem(Statement('', raw='third')), + HistoryItem(Statement('', raw='fourth')), ] ) return h @@ -72,10 +72,10 @@ def persisted_hist(): h = History( [ - HistoryItem(Statement('', raw='first'), 1), - HistoryItem(Statement('', raw='second'), 2), - HistoryItem(Statement('', raw='third'), 3), - HistoryItem(Statement('', raw='fourth'), 4), + HistoryItem(Statement('', raw='first')), + HistoryItem(Statement('', raw='second')), + HistoryItem(Statement('', raw='third')), + HistoryItem(Statement('', raw='fourth')), ] ) h.start_session() @@ -85,185 +85,158 @@ def persisted_hist(): def test_history_class_span(hist): - for tryit in ['*', ':', '-', 'all', 'ALL']: - assert hist.span(tryit) == hist - - assert hist.span('3')[0].statement.raw == 'third' - assert hist.span('-1')[0].statement.raw == 'fourth' - span = hist.span('2..') assert len(span) == 3 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('2:') assert len(span) == 3 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('-2..') assert len(span) == 2 - assert span[0].statement.raw == 'third' - assert span[1].statement.raw == 'fourth' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('-2:') assert len(span) == 2 - assert span[0].statement.raw == 'third' - assert span[1].statement.raw == 'fourth' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('1..3') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' span = hist.span('1:3') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' span = hist.span('2:-1') assert len(span) == 3 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('-3:4') assert len(span) == 3 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' span = hist.span('-4:-2') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' span = hist.span(':-2') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' span = hist.span('..-2') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' - value_errors = ['fred', 'fred:joe', 'a..b', '2 ..', '1 : 3', '1:0', '0:3'] + value_errors = ['fred', 'fred:joe', '2', '-2', 'a..b', '2 ..', '1 : 3', '1:0', '0:3'] for tryit in value_errors: with pytest.raises(ValueError): hist.span(tryit) def test_persisted_history_span(persisted_hist): - for tryit in ['*', ':', '-', 'all', 'ALL']: - assert persisted_hist.span(tryit, include_persisted=True) == persisted_hist - assert persisted_hist.span(tryit, include_persisted=False) != persisted_hist - - assert persisted_hist.span('3')[0].statement.raw == 'third' - assert persisted_hist.span('-1')[0].statement.raw == 'sixth' - span = persisted_hist.span('2..') assert len(span) == 5 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' - assert span[3].statement.raw == 'fifth' - assert span[4].statement.raw == 'sixth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' + assert span[5].statement.raw == 'fifth' + assert span[6].statement.raw == 'sixth' span = persisted_hist.span('-2..') assert len(span) == 2 - assert span[0].statement.raw == 'fifth' - assert span[1].statement.raw == 'sixth' + assert span[5].statement.raw == 'fifth' + assert span[6].statement.raw == 'sixth' span = persisted_hist.span('1..3') assert len(span) == 3 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' span = persisted_hist.span('2:-1') assert len(span) == 5 - assert span[0].statement.raw == 'second' - assert span[1].statement.raw == 'third' - assert span[2].statement.raw == 'fourth' - assert span[3].statement.raw == 'fifth' - assert span[4].statement.raw == 'sixth' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' + assert span[5].statement.raw == 'fifth' + assert span[6].statement.raw == 'sixth' span = persisted_hist.span('-3:4') assert len(span) == 1 - assert span[0].statement.raw == 'fourth' + assert span[4].statement.raw == 'fourth' span = persisted_hist.span(':-2', include_persisted=True) assert len(span) == 5 - assert span[0].statement.raw == 'first' - assert span[1].statement.raw == 'second' - assert span[2].statement.raw == 'third' - assert span[3].statement.raw == 'fourth' - assert span[4].statement.raw == 'fifth' + assert span[1].statement.raw == 'first' + assert span[2].statement.raw == 'second' + assert span[3].statement.raw == 'third' + assert span[4].statement.raw == 'fourth' + assert span[5].statement.raw == 'fifth' span = persisted_hist.span(':-2', include_persisted=False) assert len(span) == 1 - assert span[0].statement.raw == 'fifth' + assert span[5].statement.raw == 'fifth' - value_errors = ['fred', 'fred:joe', 'a..b', '2 ..', '1 : 3', '1:0', '0:3'] + value_errors = ['fred', 'fred:joe', '2', '-2', 'a..b', '2 ..', '1 : 3', '1:0', '0:3'] for tryit in value_errors: with pytest.raises(ValueError): persisted_hist.span(tryit) def test_history_class_get(hist): - assert hist.get('1').statement.raw == 'first' + assert hist.get(1).statement.raw == 'first' assert hist.get(3).statement.raw == 'third' - assert hist.get('-2') == hist[-2] + assert hist.get(-2) == hist[-2] assert hist.get(-1).statement.raw == 'fourth' with pytest.raises(IndexError): hist.get(0) - with pytest.raises(IndexError): - hist.get('0') with pytest.raises(IndexError): - hist.get('5') - with pytest.raises(ValueError): - hist.get('2-3') - with pytest.raises(ValueError): - hist.get('1..2') - with pytest.raises(ValueError): - hist.get('3:4') - with pytest.raises(ValueError): - hist.get('fred') - with pytest.raises(ValueError): - hist.get('') - with pytest.raises(TypeError): - hist.get(None) + hist.get(5) def test_history_str_search(hist): items = hist.str_search('ir') assert len(items) == 2 - assert items[0].statement.raw == 'first' - assert items[1].statement.raw == 'third' + assert items[1].statement.raw == 'first' + assert items[3].statement.raw == 'third' items = hist.str_search('rth') assert len(items) == 1 - assert items[0].statement.raw == 'fourth' + assert items[4].statement.raw == 'fourth' def test_history_regex_search(hist): items = hist.regex_search('/i.*d/') assert len(items) == 1 - assert items[0].statement.raw == 'third' + assert items[3].statement.raw == 'third' items = hist.regex_search('s[a-z]+ond') assert len(items) == 1 - assert items[0].statement.raw == 'second' + assert items[2].statement.raw == 'second' def test_history_max_length_zero(hist): @@ -301,7 +274,7 @@ def histitem(): command='help', arg_list=['history'], ) - histitem = HistoryItem(statement, 1) + histitem = HistoryItem(statement) return histitem @@ -338,7 +311,7 @@ def test_multiline_histitem(parser): assert len(history) == 1 hist_item = history[0] assert hist_item.raw == line - pr_lines = hist_item.pr().splitlines() + pr_lines = hist_item.pr(1).splitlines() assert pr_lines[0].endswith('multiline foo bar') @@ -354,7 +327,7 @@ def test_multiline_histitem_verbose(parser): assert len(history) == 1 hist_item = history[0] assert hist_item.raw == line - pr_lines = hist_item.pr(verbose=True).splitlines() + pr_lines = hist_item.pr(1, verbose=True).splitlines() assert pr_lines[0].endswith('multiline foo') assert pr_lines[1] == 'bar' @@ -375,12 +348,6 @@ def test_history_item_instantiate(): ) with pytest.raises(TypeError): _ = HistoryItem() - with pytest.raises(TypeError): - _ = HistoryItem(idx=1) - with pytest.raises(TypeError): - _ = HistoryItem(statement=statement) - with pytest.raises(TypeError): - _ = HistoryItem(statement=statement, idx='hi') def test_history_item_properties(histitem): |