Skip to content

Fix history ranges starting past the end - #1778

Open
AryanRG wants to merge 12 commits into
python-cmd2:mainfrom
AryanRG:fix/history-range-past-end
Open

AryanRG wants to merge 12 commits into
python-cmd2:mainfrom
AryanRG:fix/history-range-past-end

Conversation

@AryanRG

@AryanRG AryanRG commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

History selections with out-of-bounds starts now fail instead of selecting or replaying unintended commands. Empty action selections stop before replay, editing or file output; empty displays still succeed.

Replaying or editing persisted commands requires --all. Display and save keep full-history numbering for explicit indices and bounded ranges. The help and docs explain these rules.

Regex searches are checked before ranges, other strings are searched literally, and numeric selectors use ASCII digits. Added regression tests and updated the 4.4.0 changelog.

Validated locally with make check, make test and make docs-test on macOS / Python 3.12.

Closes #1777

@codecov

codecov Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.75%. Comparing base (1ff9293) to head (18506b6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1778   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files          23       23           
  Lines        6474     6502   +28     
=======================================
+ Hits         6458     6486   +28     
  Misses         16       16           
Flag Coverage Δ
unittests 99.75% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@tleonhardt tleonhardt left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread cmd2/history.py
Comment thread cmd2/history.py Outdated
@AryanRG

AryanRG commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

@tleonhardt Thanks, that makes sense. Empty indices and ranges now fail with No history items in range before replay, editing or file output. I also removed the redundant clamp and left persisted-history behavior unchanged for a separate discussion.

@AryanRG
AryanRG requested a review from tleonhardt October 20, 2026 05:03
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/history.py

@tleonhardt tleonhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd2/history.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/cmd2.py
Comment thread cmd2/cmd2.py
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/history.py
Comment thread cmd2/history.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/history.py Outdated
Comment thread CHANGELOG.md
Comment thread cmd2/cmd2.py Outdated

@tleonhardt tleonhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it feels like we are making progress! The edge cases I'm finding now are starting to decrease in severity I think.

Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/history.py Outdated
Comment thread cmd2/history.py
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/history.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/cmd2.py Outdated
Comment thread cmd2/cmd2.py Outdated
@AryanRG

AryanRG commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@tleonhardt Thanks for catching these. I've addressed all eight, including requiring --all to replay or edit persisted commands. Tests and docs checks pass locally.

@tleonhardt

tleonhardt commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

I think I'm happy with this now. Edge cases I can identify at this point are pretty crazy nitpicky. I'd like to wait a little before merging to see if @kmvanbrunt has any feedback and I might do some more manual testing when I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

History ranges starting past the end select and replay the last command

2 participants