-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
inconsistency in rounding days with a large increment #43
Comments
I think the the same inconsistency exists in zone aware rounding too, e.g., |
Hi @BurntSushi, thanks for using this package and for exploring this edge case. The temporal spec changed a little while ago, but the @js-temporal/polyfill package did NOT yet update to it. However, temporal-polyfill DID. So temporal-polyfill implements the newer spec for diffing/rounding. If you go to https://tc39.es/proposal-temporal/docs/ and open up the console to use the "playground" you'll see the answer is 'P2M'. The rounding logic in the spec was refactored for a number of reasons (I actually helped out with it) but one of the outcomes is that roundingIncrement only operates "within" the parent unit. So, if the unrounded result is P2M4D, then the only the 4D is being rounded, within 0-35, and of course it rounds down to 0. But I'm glad you brought this up because rounding UP could yield strange results. I've opened a TC39 ticket here: tc39/proposal-temporal#2902 |
Oh nice! That's a relief. It is definitely an odd case and I wasn't fully sure what the right answer ought to be, but what y'all settled on seems sensible. And thanks for the tip on using the console on the doc web site.
Yeah I've been following this and really appreciated the gist you provided. It was quite timely too, because I was dreading implementing duration rounding before then. But your approach massively simplified it. |
No problem at all.
That makes me very happy, I'm glad others got some good use out of that! |
I have this code snippet:
Using this polyfill, the output is
2m
. But with@js-temporal/polyfill
, I'm getting2m9d
. I think the latter is correct. From what I can tell, the issue is here:temporal-polyfill/packages/temporal-polyfill/src/internal/round.ts
Lines 538 to 544 in 2d3fd89
Namely, this is rounding the duration based only on the time/day component. But it is neglecting the days contributed by units higher than days. Which in this case is 2 months since the duration, at this point, has been balanced.
(I am working on a datetime library for Rust inspired by Temporal, and I found this case by trying weird values of
roundingIncrement
.)The text was updated successfully, but these errors were encountered: