Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1778 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6474 6487 +13
=======================================
+ Hits 6458 6471 +13
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
@AryanRG Thanks for the PR!
While I think the 1-line change in cmd2/history.py in this PR is a clever solution for handling the problem where a range past the end of the history inappropriately treating that range as equal to the last command, I think as-is this results in some behavior which feels off an non-intuitive.
As a user, I would expect and prefer the command to fail fast with a clear error message informing me that I have selected an invalid range instead of it silently behaving in a way that at least for some history commands is non-intuitive using an empty-range.
For example, take history -r 5: with only 3 items now does nothing and still reports success. -e opens an empty editor and -o writes an empty file. Before this branch, the same input acted on the last command, which also feels inappropriate.
I think the user should now get an error such as "No history items in range" instead of using an empty range. Fixing this will likely require an edit to cmd2.py somewhere around line 5505. This overall review comment is the important one. The two others in my review are minor.
…/history-range-1778 # Conflicts: # CHANGELOG.md
|
@tleonhardt Thanks, that makes sense. Empty indices and ranges now fail with |
tleonhardt
left a comment
There was a problem hiding this comment.
The bounds checks live in three places: the integer-index path in _get_history, the IndexError from span(), and the exemption for ranges with no start.
I suggest having History.get() raise the same IndexError("No history items in range"), or better yet a custom exception subclass derived from this, and catching it once in _get_history. That should also fix the history 0 problem, and other callers of get() would get a clear message instead of "list index out of range".
The history command is by far the most complex command built into cmd2. There are a LOT of features which results in a LOT of possible edge cases.
| history = self._get_history(args) | ||
|
|
||
| if history is None: | ||
| return None |
There was a problem hiding this comment.
Empty selections still overwrite files.
A bare : range (no start or end) that selects nothing still runs the action. In a fresh session, history -o notes.txt : overwrites notes.txt with nothing, and history -e : opens an empty editor and runs whatever is typed.
The branch's goal is to refuse empty selections before running, editing or saving, but test_history_unbounded_empty_actions locks this behaviour in.
There was a problem hiding this comment.
Fixed, you were right. Empty selections now stop before replay, editing or file output, including unbounded ranges and search misses. Empty history display still works.
History ranges starting past the end no longer select the last command. Invalid indices, including zero, and bounded ranges selecting no items report
No history items in rangebefore replay, editing or file output.Empty action selections also stop before any side effects, including unbounded ranges, omitted selectors and search misses. Displaying an empty selection still succeeds, and the no-argument
history --runsafety refusal is unchanged.History.get()andspan()raiseHistoryIndexError, caught once by_get_history(). UnexpectedIndexErrors are not swallowed. Valid negative-range clamping and persisted-history selections are preserved. The API change is documented under 4.4.0.Added regression coverage, including persisted-file reloads.
make check,make testandmake docs-testpass locally on macOS / Python 3.12. Hosted CI is rerunning.Closes #1777