fix: support negative indices in OrderedSet.__getitem__#633
Conversation
Negative index access (e.g. ``s[-1]``) previously raised ``ValueError`` from ``itertools.islice``, which forbids negative start values. This is surprising because Python sequences uniformly accept negative indices (``s[-1]`` returns the last element) and the custom ``__getitem__`` on ``OrderedSet`` already raises ``IndexError`` for positive out-of-range indices. Normalise the index before passing it to ``islice``: add ``len(self)`` when ``index < 0``, and raise ``IndexError`` (not ``ValueError``) when the normalised index is still negative. The original index is preserved in the error message so the reported value matches what the caller passed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #633 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 52 52
Lines 5494 5499 +5
Branches 865 867 +2
=========================================
+ Hits 5494 5499 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The previous CI run failed on a timing-sensitive flaky test unrelated to this PR's changes: `FAILED tests/test_contrib_timeout.py::TestTimeoutComposition::test_timeout_fires_before_slow_invoke[async]` The assertion `assert 'stuck' in OrderedSet(['loading'])` failed because the 50ms timeout didn't fire within the CI window. The test passes consistently locally. I've pushed a minor fixup commit (removing extra blank lines in the docstring) to re-trigger CI. Could you approve the workflow run so CI can run again? |
|
Hi @gaoflow, thanks for the careful fix and tests! Verified locally: tests and doctests pass, and the new branches in |
|
2 participants
What
OrderedSet.__getitem__raisedValueError(fromitertools.islice) when called with a negative index, instead of theIndexErrorcallers expect — or, better, returning the element counting from the end, as all Python sequences do.Why
itertools.islicerejects negative start/stop values withValueError. Since__getitem__is documented and exported as a sequence-style accessor (positive indices already work, positive out-of-range already raisesIndexError), negative indices should follow the same contract as Python's built-in sequences.Fix
Normalise the index before delegating to
islice:index < 0, addlen(self)to convert it to the equivalent positive offsetIndexErrorwith the original index in the message (not the normalised value)Two new unit tests cover the happy path and the out-of-range case. Doctests in
orderedset.pyare updated with examples ofs[-1]ands[-3].This pull request was prepared with the assistance of AI, under my direction and review.