Repository navigation
Truncate fractional seconds per UTS 35 - #1292
Conversation
d4e3c2b to
dbd44a5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1292 +/- ##
=======================================
Coverage 93.57% 93.57%
=======================================
Files 27 27
Lines 4951 4952 +1
=======================================
+ Hits 4633 4634 +1
Misses 318 318
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:
|
|
According to https://www.unicode.org/reports/tr35/tr35-dates.html#dfst-second, it says Fractional Second (numeric). Truncates, like other numeric time fields, but in this case to the number of digits specified by the field length. So, the The document of Date and Time in Babel, https://babel.pocoo.org/en/latest/dates.html#time-fields, says Fractional second, rounds to the count of letters, and the document should be fixed too. |
|
You are right, and that is the better fix: TR35 says the field truncates, and truncation makes the overflow impossible rather than clamping it after the fact. Two things worth deciding before I switch this PR over, since truncating is a wider change than the overflow repair. It changes three expectations that
The other two cases in that test, and The arithmetic is also better done on the integer microseconds than on the float: return self.format(self.value.microsecond // 10 ** (6 - num), num) # num <= 6
Happy to push that as this PR, updating the three expectations and the docstring and dropping the clamp, if you would rather have the spec-correct behaviour than the narrower overflow fix. Say the word and I will rework it. |
|
Friendly bump: still relevant, and it merges cleanly with the base branch. Happy to adjust anything. |
We would rather have the spec-correct behaviour, yes. :) |
dbd44a5 to
f3e47e5
Compare
|
Switched to truncation per UTS #35, rebased on current master, and updated the docs and tests. The overflow case is still covered by a regression test. |
vineethsaivs
changed the title
format_frac_seconds rounded microseconds, which could carry to a whole second and overflow the field (S gave 10). UTS python-babel#35 says to truncate to the field length, so it now truncates with integer math. Docs and expectations updated.
f3e47e5 to
4a0d92d
Compare
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
LGTM.
https://www.unicode.org/reports/tr35/tr35-dates.html#dfst-second plainly says
Fractional Second (numeric). Truncates, like other numeric time fields, but in this case to the number of digits specified by the field length. (Example shows display using pattern SSSS for seconds value 12.34567)
and it looks like that's what we do now.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain, and the fractional-second formatting change includes corresponding regression tests and documentation updates.
No open issues 🎉
What changed in this PR
Updates fractional-second formatting to comply with UTS #35 by truncating microseconds instead of rounding, preventing values from carrying into the next whole second.
Changes:
- Replaced rounding-based formatting with integer truncation and zero-padding.
- Updated tests and added regression coverage for high-precision values.
- Documented truncation behavior for fractional-second fields.
| File | Description |
|---|---|
tests/test_date_time_format.py |
Updates expected truncation results and adds carry-over regression tests. |
docs/dates.rst |
Documents truncation semantics for the S field. |
babel/dates.py |
Implements truncation-based fractional-second formatting. |
Review effort: Lite
format_frac_secondsrounded microseconds, which could carry to a whole second and overflow the field (Sgave10). UTS #35 says to truncate to the field length, so it now truncates with integer math, which also removes the overflow. The docs and the three rounding expectations are updated, and a 999999 regression test coversS,SS,SSSSandSSSSSSSS.