Fix format_timespan() rounding the smallest unit past a whole unit of the next one - #86
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
format_timespan() rounds the display of its smallest shown unit to two decimal places while every coarser unit is computed by truncation. When the remaining fraction was close enough to round up to a whole unit, the result could equal or exceed a full unit of the next larger (already rendered) unit, e.g. format_timespan(119.999) produced "1 minute and 60 seconds" instead of "2 minutes", and format_timespan(3599.999) produced "59 minutes and 60 seconds" instead of "1 hour". Fix this by rounding the whole timespan (in seconds) to the precision that will end up being displayed for the smallest unit before splitting it into units, so the usual truncating division naturally carries the extra unit upward instead of stranding an out of range count on the smallest unit. Added a regression test covering both the plain and detailed rendering of the previously broken cases.
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
1 participant
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.
When `format_timespan()` is close to a rollover point, the smallest unit it decides to show gets rounded to two decimal places for display, but every larger unit before it is computed by plain truncation. If the leftover fraction is close enough to round all the way up, the rounded count can equal or exceed a whole unit of the coarser value that was already emitted.
A couple of reproductions on the current release:
```pycon
Neither of those is a valid decomposition, "60 seconds" should have carried into the minute (or hour) count.
The fix rounds the total number of seconds to the precision that will end up being shown for the smallest active unit before the per-unit loop runs, instead of rounding only the smallest unit's own count afterwards. That way the existing truncating division for the coarser units naturally absorbs the carry, and no unit ever displays a count that belongs to the next unit up:
```pycon
I checked this against the existing test suite (all of the `format_timespan`/`parse_timespan`/rounding related cases still pass, including the fixes from issues #10 and #11) and ran a 200k-sample fuzz over random timespans in both detailed and non-detailed mode asserting no unit's rendered count reaches or exceeds the bound of the next coarser unit; it came back clean with the fix applied (and reproduces failures without it).
Added a regression test for the two cases above plus the detailed-mode equivalent.