Fix ordinal() suffix for negative integers#321
Open
fbindakheel wants to merge 1 commit into
Open
Conversation
ordinal() selected the suffix using value % 10 and value % 100, but Python's modulo on negative numbers is non-negative (e.g. -1 % 10 == 9), so negative inputs got the wrong suffix: ordinal(-1) returned '-1th' instead of '-1st', ordinal(-21) returned '-21th' instead of '-21st', etc. The docstring states it works for any integer, so compute the suffix from abs(value). Add regression tests for negative ordinals.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 12 12
Lines 900 901 +1
=======================================
+ Hits 896 897 +1
Misses 4 4
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:
|
Member
Strictly speaking maybe, but perhaps the original intent was for "any positive integer"? Stepping back a moment: do negative ordinals like -1st, -2nd or -21st even make sense? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
ordinal()returns the wrong suffix for negative integers:The docstring states the function "works for any integer," so negatives are in scope.
Root cause
The suffix is selected with
value % 10/value % 100. Python's modulo on negative numbers returns a non-negative result (-1 % 10 == 9), so negative values index into the wrong_ORDINAL_SUFFIXESslot (9→"th"). Some negatives (e.g.-11,-12) happen to land on"th"correctly by coincidence, but-1,-2,-3,-21,-101, etc. are wrong.Fix
Compute the suffix from
abs(value). The displayed number keeps its sign. One-line logic change.Tests
Added negative cases to the
test_ordinalparametrization (-1→-1st,-2→-2nd,-3→-3rd,-11→-11th,-21→-21st,-101→-101st,-111→-111th, etc.). Verified the new cases fail on the current code and pass with the fix; full suite (696 tests) passes.