Round ISO8601 fractional seconds to the nearest millisecond#1997
Round ISO8601 fractional seconds to the nearest millisecond#1997adityasingh2400 wants to merge 2 commits into
Conversation
When formatting with includingFractionalSeconds, the milliseconds field
was computed from the nanosecond component by truncating toward zero:
let ms = Int((Double(ns) / 1_000_000.0).rounded(.towardZero))
A Date created from a fractional TimeInterval at a present-day magnitude
cannot represent the value exactly. For Date(timeIntervalSince1970:
1674036251.123) the nanosecond component the calendar extracts is
122999906 rather than 123000000, so truncation yields ".122" instead of
".123". The same off-by-one drops the last millisecond for many values,
for example ".001" becomes ".000" and ".011" becomes ".010".
Round to the nearest millisecond instead. The result is clamped to 999
so a value that rounds up to 1000 does not overflow the three-digit
field, matching the existing behavior already covered by the rounding()
test for ".9999".
Fixes swiftlang#963.
|
Does this work for the scenario that @parkera mentioned in his comment in the issue?
Can we add a unit test to see what this change would do to this input? |
Cover the scenario from issue swiftlang#963: a time at HH:MM:59 with a sub-millisecond remainder that rounds up to 1000 ms must clamp to .999 and keep the second at 59, never reading .000 or carrying into the seconds field.
|
Yes, it handles that case. The min(..., 999) clamp is exactly the guard for it: when the sub-millisecond remainder rounds up to 1000 ms, it is capped at .999 rather than wrapping the rounded value to .000 or carrying into the seconds field. So a time like 12:59:59.9999 formats as 12:59:59.999, never 12:59:59.000. I added a test at the :59 boundary in 7208ae6 (1674036299.0 + 0.9996 is 2023-01-18T10:04:59) asserting the output stays at .999. The existing per-millisecond loop and the round-up-to-1000 test cover the rest. |
Date.ISO8601FormatStylewithincludingFractionalSecondsreports the milliseconds field one millisecond too low for many values. For example:The same off-by-one drops the last millisecond for a large fraction of inputs at present-day magnitudes, e.g.
.001formats as.000and.011formats as.010.Root cause: the milliseconds field is derived from the nanosecond component by truncating toward zero.
A
Datebuilt from a fractionalTimeIntervalat a current-date magnitude cannot represent the value exactly, and the nanosecond the calendar extracts lands just below the intended value. For the example above the nanosecond component is122999906rather than123000000, so truncating gives122. This only shows up at large magnitudes: the existing665076946.011test case happens to land on a nanosecond at or above11000000, so truncation still produced.011there, which is why it was not caught.The fix rounds to the nearest millisecond instead of truncating, and clamps the result to
999so a value that would round up to1000does not overflow the three-digit field. That clamp keeps the behavior already asserted by the existingrounding()test, where15:35:45.9999formats as15:35:45.999.Parsing is unaffected: it already maps
.123back to123000000ns exactly, so the formatted output now matches what the parser expects for the displayable resolution.Verification: added a regression test that fails before this change and passes after. It checks the reported
.123case, asserts that every millisecond0..<1000from a present-day base round-trips through the formatted string, and checks the1000overflow clamp. The fullISO8601FormatStyle,Date.FormatStyle, and HTTP format style suites pass (the only failure in the widerFoundationEssentialsTestsrun isfileExtendedAttributes, which also fails on a clean checkout and is unrelated).Fixes #963.
Happy to retarget this at an active release branch if that fits the merge-forward strategy better.