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: Disallow rounding to increment while balancing to calendar unit #2916

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jul 15, 2024

The corner case of rounding to a >1 increment of a calendar smallest unit while simultaneously balancing to a larger calendar unit is ambiguous. This use case was probably never considered.

const d1 = Temporal.Duration.from({months: 9});
d1.round({
  relativeTo: '2024-01-01',
  largestUnit: 'years',
  smallestUnit: 'months',
  roundingIncrement: 8,
  roundingMode: 'ceil',
});  // => 1 year? 1 year 4 months?

This never came up in real-world usage. Disallow it explicitly, to leave space for a future proposal if it ever comes up.

Closes: #2902

ptomato added a commit to ptomato/test262 that referenced this pull request Jul 15, 2024
This test covers a normative change to disallow an ambiguous corner case
in the options bag for Temporal.Duration.prototype.round().

See tc39/proposal-temporal#2916
@ptomato ptomato marked this pull request as draft July 15, 2024 23:54
@ptomato
Copy link
Collaborator Author

ptomato commented Jul 15, 2024

Draft until presented to TC39.

@ptomato
Copy link
Collaborator Author

ptomato commented Jul 15, 2024

Test262 update in tc39/test262#4152. Note tests are currently passing on this PR because this case just wasn't covered.

@ptomato ptomato mentioned this pull request Jul 15, 2024
91 tasks
@ptomato ptomato marked this pull request as ready for review July 30, 2024 22:19
@ptomato
Copy link
Collaborator Author

ptomato commented Jul 30, 2024

Achieved consensus at TC39 plenary 2024-07-30.

Copy link
Collaborator

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

👍

… unit

The corner case of rounding to a >1 increment of a calendar smallest unit
while simultaneously balancing to a larger calendar unit is ambiguous.
This use case was probably never considered.

const d1 = Temporal.Duration.from({months: 9});
d1.round({
  relativeTo: '2024-01-01',
  largestUnit: 'years',
  smallestUnit: 'months',
  roundingIncrement: 8,
  roundingMode: 'ceil',
});  // => 1 year? 1 year 4 months?

This never came up in real-world usage. Disallow it explicitly, to leave
space for a future proposal if it ever comes up.

Closes: #2902
ptomato added a commit to ptomato/test262 that referenced this pull request Sep 5, 2024
This test covers a normative change to disallow an ambiguous corner case
in the options bag for Temporal.Duration.prototype.round().

See tc39/proposal-temporal#2916
ptomato added a commit to tc39/test262 that referenced this pull request Sep 5, 2024
This test covers a normative change to disallow an ambiguous corner case
in the options bag for Temporal.Duration.prototype.round().

See tc39/proposal-temporal#2916
@ptomato ptomato force-pushed the 2902-disallow-calendar-rounding-increment branch from 3796b9b to 6f175b5 Compare September 5, 2024 22:09
@ptomato ptomato merged commit ecd55b5 into main Sep 5, 2024
5 checks passed
@ptomato ptomato deleted the 2902-disallow-calendar-rounding-increment branch September 5, 2024 22:13
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.

RoundRelativeDuration: Disallow edge case in roundingIncrement within year/month
2 participants