Skip to content

Fix precisedelta rounding overflow across suppressed units - #417

Open
afonsojanu wants to merge 1 commit into
python-humanize:mainfrom
afonsojanu:fix/precisedelta-suppressed-unit-overflow
Open

afonsojanu wants to merge 1 commit into
python-humanize:mainfrom
afonsojanu:fix/precisedelta-suppressed-unit-overflow

Conversation

@afonsojanu

Copy link
Copy Markdown

precisedelta() has an overflow-correction step for when rounding pushes the minimum unit's displayed value up to (or past) the next unit's threshold, e.g. rounding 59.9 minutes to 60 minutes gets turned into "1 hour". That correction only ever looks one unit up, though, and gives up entirely if that neighbouring unit happens to be in suppress:

>>> import datetime as dt
>>> import humanize
>>> d = dt.timedelta(seconds=59, microseconds=999937)
>>> humanize.precisedelta(d, minimum_unit="milliseconds", format="%0.0f")
'1 minute'
>>> humanize.precisedelta(d, minimum_unit="milliseconds", suppress=["seconds"], format="%0.0f")
'60000 milliseconds'

Both calls represent the exact same duration, and 999937 microseconds rounds to 1000 milliseconds under %0.0f, which is exactly what the existing carry logic is meant to catch. It works fine when seconds isn't suppressed, but as soon as it is, the carry has nowhere to go (the check that's supposed to promote it into minutes is gated on SECONDS not in suppress_set), so the rounding artifact just sits there as an invalid "60000 milliseconds" instead of folding into the next unit that's actually shown.

It compounds across more than one suppressed unit too:

>>> d = dt.timedelta(seconds=3599, microseconds=999900)
>>> humanize.precisedelta(d, minimum_unit="milliseconds", suppress=["seconds", "minutes"], format="%0.0f")
'3600000 milliseconds'   # should be '1 hour'

I changed the carry to keep compounding the conversion factor through any suppressed units above the minimum unit until it reaches one that's actually displayed, then applies the carry there. Units that legitimately hold a large count because a higher unit was suppressed (like 308 hours when days is suppressed) are unaffected, since a large value like that never reaches the compounded threshold — it's only the narrow rounding-boundary case that gets fixed.

Added regression tests covering the single-suppressed-unit case, the two-level compounding case, and one confirming the legitimate "large value from a suppressed higher unit" case is untouched. The full existing suite still passes (392 tests in test_time.py), along with ruff, ruff format --check, and mypy --strict. I also differential-fuzzed precisedelta() against the pre-fix implementation across ~400k randomized inputs (including values placed right at unit boundaries and various suppress/minimum_unit combinations) and saw zero behavioral differences outside of the two scenarios this fixes.

When a suppressed unit sits between the minimum_unit and the nearest
unit that is still shown, an overflow caused by rounding the
minimum_unit's value (e.g. 999.9999 milliseconds rounding up to 1000)
had nowhere to carry into: the existing carry check only looked one
level up and gave up as soon as that immediate neighbour was itself
suppressed.

For example, with seconds suppressed and milliseconds as the minimum
unit, a value just under a full minute could render as "60000
milliseconds" instead of "1 minute", even though the exact same
duration without suppression correctly rounds to "1 minute".

The carry now compounds the conversion factor across any suppressed
units until it reaches one that is actually displayed, matching how
the unsuppressed case already behaves. Values that legitimately hold a
large number in a unit because a higher unit was suppressed (e.g. 308
hours when days are suppressed) are unaffected, since they never reach
the compounded threshold.

Added regression tests for the single- and double-suppressed-unit
carry, plus one confirming the legitimate large-value case is left
untouched. Ran the existing suite (392 tests in test_time.py, 735
passing overall), ruff, ruff format and mypy --strict with no new
issues, and differential-fuzzed precisedelta() against the previous
implementation across 400k randomized inputs (uniform, boundary-
adjacent, and multi-unit-suppressed combinations) with zero
behavioral differences outside of this fix.
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.

1 participant