Fix precisedelta rounding - #254
Conversation
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 99.49% 99.51% +0.01%
==========================================
Files 11 11
Lines 794 820 +26
==========================================
+ Hits 790 816 +26
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
%d and %0.0f do not produce the same values. So we will first apply the required formatting and turn the formatted string back into a float or int. YEARS needs to be treated slightly differently as it needs to be formatted with `intcomma`. We first check if the resulting value does not have any fractional part and if not, we turn it into an int, so that it string output is what a human would expect. Added several unittests to highlight some of the differences between using %d and %.0f as a format in precisedelta.
We only try to round the value based on the format provided if we are dealing with the minimum_unit.
|
I need a bit more time. I think I can even fix #30 with this PR. |
Sorry, something went wrong.
|
I added one more commit to address #30 issue. Unittests hopefully describe what is the expected behaviour. I had to make some decisions for corner cases. I'm happy to discuss if there is a disagreement on any of this. |
Sorry, something went wrong.
The logic about rounding due to formatting has been moved to _quotient_and_remainder because this is where we have all the logic regarding the minimum unit and suppress units. Also removed _carry function. Instead use the same logic than was used for calculating the `secs` based on the remaining amount of days. It is done now also for `usecs` based on the `secs` remaining. Add another block of logic to check after rounding if any units should be promoted to a higher unit, in case of a rounding up.
| (dt.timedelta(days=31), "seconds", "1 month and 12 hours"), | ||
| (dt.timedelta(days=32), "seconds", "1 month, 1 day and 12 hours"), | ||
| (dt.timedelta(days=62), "seconds", "2 months and 1 day"), | ||
| (dt.timedelta(days=92), "seconds", "3 months and 12 hours"), | ||
| (dt.timedelta(days=31), "days", "1 month and 0.50 days"), | ||
| (dt.timedelta(days=32), "days", "1 month and 1.50 days"), | ||
| (dt.timedelta(days=62), "days", "2 months and 1 day"), | ||
| (dt.timedelta(days=92), "days", "3 months and 0.50 days"), |
There was a problem hiding this comment.
Thanks for the PR!
I know we use 30.5 days = month for some approximations but I think this extra 12 hours may be unexpected?
import datetime as dt
from humanize.time import precisedelta
for days in (30, 31, 32):
print(f"{days} -> {precisedelta(dt.timedelta(days=days))}")Before:
30 -> 30 days 31 -> 1 month and 0 days 32 -> 1 month and 1 days
After:
30 -> 30 days 31 -> 1 month and 12 hours 32 -> 1 month, 1 day and 12 hours
Sorry, something went wrong.
There was a problem hiding this comment.
I see what you mean and I understand it's indeed confusing. I took it a bit too literally that one month is 30.5 days.
I pushed a new commit where we only consider months to be 30.5 days on average. We round down any remainder when we try to determine the number of months. This means that 31 days is 1 month and 61 days is 2 months.
Sorry, something went wrong.
Although 1 month is 30.5 days on average, we want 31 days to be one month, and not remainder of 0.5 days which would not be intuitive.
|
@hugovk In case you find a bit of time, do you like the new approach I took? |
Sorry, something went wrong.
|
Thanks for the reminder! Let's ping some of the people from the other issues/PRs to give them a few days to give feedback, otherwise let's go for it. @ddellspe @eldipa @nuztalgia @shazib-summar @xk4rimx Any comment on this? |
Sorry, something went wrong.
|
This looks good to me, at least resolves the issue I was seeing. |
Sorry, something went wrong.
|
yea. lgtm. thanks. |
Sorry, something went wrong.
|
Thanks all! |
Sorry, something went wrong.
All 10 projects validated: verify.sh FAILs on BEFORE commit, passes on AFTER. Python projects (8): - real-dotenv-cwd-bug: Fix CLI crash when CWD deleted (theskumar/python-dotenv#446) - real-humanize-precisedelta: Fix rounding carry-over in precisedelta (python-humanize/humanize#254) - real-humanize-intword: Fix plural form and boundary rounding in intword (python-humanize/humanize#273) - real-iniconfig-parse: Add IniConfig.parse() with inline comment stripping (pytest-dev/iniconfig#70) - real-itsdangerous-overflow: Handle date overflow in timed unsign (pallets/itsdangerous) - real-pathspec-empty: Fix ValueError for empty pattern lists in backends (cpburnz/python-pathspec#100) - real-precommit-empty-sort: Fix file-contents-sorter adding blank line to empty files (pre-commit/pre-commit-hooks#935) - real-precommit-debug-bpdb: Add bpdb debugger detection (pre-commit/pre-commit-hooks#942) Node.js projects (2): - real-yargs-parser-hyphens: Fix parsing of options ending with 3+ hyphens (yargs/yargs-parser#434) - real-string-width-emoji: Fix width for minimally-qualified emoji sequences (sindresorhus/string-width#68) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes #14
Fixes #20
Fixes #30
Based on PR #39 .
Changes proposed in this pull request: