Skip to content
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

Normative: Correctly respect largestUnit option in duration rounding #2571

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented May 3, 2023

Note: this PR is stacked on top of #2567 and #2519. The only relevant commit for this issue is the topmost one, "Normative: Balance durations after rounding in until()/since()/toString()".

In cases where a number rounded up to the next unit, we would previously return a result that didn't respect the largestUnit option in the same way that Duration.prototype.round() would.

To fix this, we need to call BalanceDateDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly).

In Duration.prototype.toString() where time units did not already behave correctly, we need to call BalanceDuration. I was not sure whether it was intentional that we didn't call this, but as far as I can determine from in toString() was "controls rounding, works same as round()", but the status quo was that it worked differently from round().)

Test262 tests are in a branch which is also stacked on top of tests for #2519. I will make it into a PR after we have merged #2519. Tests are in tc39/test262#3956

Closes: #2563

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ee62aee) 96.36% compared to head (652788d) 96.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2571      +/-   ##
==========================================
+ Coverage   96.36%   96.38%   +0.02%     
==========================================
  Files          21       21              
  Lines       12535    12615      +80     
  Branches     2345     2356      +11     
==========================================
+ Hits        12079    12159      +80     
  Misses        401      401              
  Partials       55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch from 4460151 to f631d18 Compare May 3, 2023 22:16
@ptomato ptomato marked this pull request as draft May 3, 2023 23:52
@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch 3 times, most recently from 83ccaf4 to 282dd60 Compare May 13, 2023 00:56
@ptomato ptomato marked this pull request as ready for review May 17, 2023 23:12
@ptomato
Copy link
Collaborator Author

ptomato commented May 17, 2023

This change achieved consensus today at the May 2023 TC39 plenary meeting. Due to the PR being stacked on top of #2519, though, it would be better to merge this after merging that one first.

@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch from 282dd60 to e1699bc Compare May 19, 2023 17:50
@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch from e1699bc to 799f08a Compare May 31, 2023 20:08
@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch from 799f08a to 7dc976d Compare June 12, 2023 11:41
@ptomato ptomato force-pushed the 2563-duration-rounding-bug branch from 7dc976d to ff9c35d Compare June 20, 2023 11:36
@ptomato ptomato mentioned this pull request Jul 13, 2023
91 tasks
@justingrant
Copy link
Collaborator

Is this PR now part of #2645?

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 14, 2023

Is this PR now part of #2645?

No, it won't be able to land until all of #2519 has landed. (I didn't/don't want to write a separate version of this PR to work correctly in the pre-Calendar-Method-Record world.)

Copy link
Collaborator

@cjtenny cjtenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me like it correctly does what it says on the tin - with one potential bug to address, see comment.

spec/plaindatetime.html Show resolved Hide resolved
@jasonwilliams
Copy link
Member

jasonwilliams commented Nov 8, 2023

To fix this, we need to call BalanceDurationRelative after RoundDuration in the until()/since() methods that deal with date units (time units already behaved correctly).

Did you mean to say "we need to call BalanceDateDurationRelative" here?

PR looks good to me, I've spoken offline about potentially merging some logic into roundDuration but that will need some big changes including more parameters so may not be worth it for now.

@ptomato
Copy link
Collaborator Author

ptomato commented Nov 9, 2023

Did you mean to say "we need to call BalanceDateDurationRelative" here?

Indeed, thanks! Fixed.

…ng()

In cases where a number rounded up to the next unit, we would previously
return a result that didn't respect the `largestUnit` option in the same
way that Duration.prototype.round() would.

To fix this, we need to call BalanceDateDurationRelative after
RoundDuration in the until()/since() methods that deal with date units
(time units already behaved correctly).

In Duration.prototype.toString() where time units did not already behave
correctly, we need to call BalanceTimeDuration. I was not sure whether it
was intentional that we didn't call this, but as far as I can determine
from the previous discussions, `toString()` was "controls rounding, works
same as `round()`", but the status quo was that it worked differently from
`round()`.)

Closes: #2563
@ptomato
Copy link
Collaborator Author

ptomato commented Nov 14, 2023

Tests are merged now, so this can be merged.

@ptomato ptomato merged commit c040562 into main Nov 14, 2023
9 checks passed
@ptomato ptomato deleted the 2563-duration-rounding-bug branch November 14, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different duration balancing PlainDate::until vs Duration::round
4 participants