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: Upper limit on rounding increments #2480

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jan 17, 2023

Rounding increments are usually limited. In a few cases (years, months, weeks, and days for Temporal.Durations), they were previously only limited by being required to be finite. This introduces a limit of 1e9 for these previously unlimited cases.

1e9 fits in a 32-bit integer, to make things simpler for implementations, but is clearer to explain in end-user documentation than UINT32_MAX.

Closes: #2458

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #2480 (2e091af) into main (98607ce) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 2e091af differs from pull request most recent head 2728067. Consider uploading reports for the commit 2728067 to get more accurate results

@@            Coverage Diff             @@
##             main    #2480      +/-   ##
==========================================
- Coverage   94.97%   94.95%   -0.02%     
==========================================
  Files          20       20              
  Lines       10903    10825      -78     
  Branches     1996     1959      -37     
==========================================
- Hits        10355    10279      -76     
  Misses        487      487              
+ Partials       61       59       -2     
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.29% <100.00%> (-0.01%) ⬇️
polyfill/lib/zoneddatetime.mjs 99.82% <0.00%> (+<0.01%) ⬆️
polyfill/lib/plaindate.mjs 99.66% <0.00%> (+<0.01%) ⬆️
polyfill/lib/plainyearmonth.mjs 98.36% <0.00%> (+<0.01%) ⬆️
polyfill/runtest262.mjs 100.00% <0.00%> (+10.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ptomato added a commit to ptomato/test262 that referenced this pull request Jan 17, 2023
Previously in a few cases (calendar units in Duration) the value for the
roundingIncrement option had no upper limit, other than having to be
finite. These tests cover a normative change limiting it to 1e9.

Normative PR: tc39/proposal-temporal#2480
Comment on lines -210 to +212
1. If _increment_ &lt; *1*<sub>𝔽</sub>, throw a *RangeError* exception.
1. Return truncate(ℝ(_increment_)).
1. Let _integerIncrement_ be truncate(ℝ(_increment_)).
1. If _integerIncrement_ &lt; 1 or _integerIncrement_ &gt; 10<sup>9</sup>, throw a *RangeError* exception.
1. Return _integerIncrement_.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gibson042 I thought you might be particularly interested to note this change since you did an earlier similar change to check the range for fractionalSecondDigits after truncating it; now that we have an upper limit, it matters here too.

@ptomato
Copy link
Collaborator Author

ptomato commented Jan 17, 2023

Tests for this are in tc39/test262#3770

spec/abstractops.html Outdated Show resolved Hide resolved
Rounding increments are usually limited. In a few cases (years, months,
weeks, and days for Temporal.Durations), they were previously only limited
by being required to be finite. This introduces a limit of 1e9 for these
previously unlimited cases.

1e9 fits in a 32-bit integer, to make things simpler for implementations,
but is clearer to explain in end-user documentation than UINT32_MAX.
ptomato added a commit to ptomato/test262 that referenced this pull request Feb 1, 2023
Previously in a few cases (calendar units in Duration) the value for the
roundingIncrement option had no upper limit, other than having to be
finite. These tests cover a normative change limiting it to 1e9.

Normative PR: tc39/proposal-temporal#2480
ptomato added a commit to tc39/test262 that referenced this pull request Feb 1, 2023
Previously in a few cases (calendar units in Duration) the value for the
roundingIncrement option had no upper limit, other than having to be
finite. These tests cover a normative change limiting it to 1e9.

Normative PR: tc39/proposal-temporal#2480
@ptomato ptomato force-pushed the 2458-limit-rounding-increment branch from 2e091af to 2728067 Compare February 1, 2023 19:51
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 1, 2023

This change reached consensus at the 2023-01-31 TC39 plenary meeting. Test262 tests have already been merged.

@ptomato ptomato merged commit 0bb12c2 into main Feb 1, 2023
@ptomato ptomato deleted the 2458-limit-rounding-increment branch February 1, 2023 19:56
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.

Proposal to limit rounding increments for hour, month, week, and day
3 participants