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: Limit valid values for DurationFormats to match upcoming limits in Temporal #173

Merged
merged 1 commit into from May 23, 2024

Conversation

ben-allen
Copy link
Collaborator

See #157

Upcoming revisions to Temporal will limit the valid values for Temporal.Duration. This commit applies the same limits on valid values for Intl.DurationFormat DurationRecords. Specifically: the absolute values of the values stored in each of the [[Years]], [[Months]], and [[Weeks]] fields cannot exceed 2^32, and the absolute value of the value obtained through converting the values in each of the sub-week fields to seconds and then summing them must not exceed 2^53.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I'm not involved enough with this proposal to approve the change, but the limits are aligned with the normative change that was approved in Temporal.

@sffc sffc requested a review from FrankYFTang October 24, 2023 03:10
spec.emu Outdated Show resolved Hide resolved
@sffc sffc added consensus We reached a consensus in a discussion meeting, through email or the issue discussion needs-tg1 labels Nov 16, 2023
@ryzokuken
Copy link
Member

2023-11-28: This PR achieved TC39-TG1 consensus.

@anba
Copy link
Contributor

anba commented Jan 12, 2024

Test coverage → tc39/test262#3988

@ben-allen
Copy link
Collaborator Author

Updated to match limits currently set in Temporal. See: tc39/proposal-temporal#2727

@ptomato has pointed me toward tc39/proposal-temporal#2727 (comment) for thoughts on implementation. He's also written a polyfill that implements the limits via string manipulation to run test262 tests against.

@ryzokuken @FrankYFTang

@ptomato
Copy link
Contributor

ptomato commented May 7, 2024

Note the text in Temporal PR 2727 was not the latest, Anba made some changes to it afterwards.

@ben-allen
Copy link
Collaborator Author

Note the text in Temporal PR 2727 was not the latest, Anba made some changes to it afterwards.

Just pushed a corrected version. Thanks for catching that!

@syg
Copy link

syg commented May 15, 2024

Does this change mean BigInt math is not required (just like in Temporal)?

@FrankYFTang
Copy link
Collaborator

FrankYFTang commented May 15, 2024

Could we do the following change to better match Temporal?

  1. in ToDurationRecord change
24. If IsValidDurationRecord(result) is false, throw a RangeError exception.

to

24. If IsValidDuration( result.[[Years]] ,  result.[[Months]] ,  result.[[Weeks]] ,  result.[[Days]] ,  result.[[Hours]] ,  result.[[Minutes]] ,  result.[[Seconds]] ,  result.[[Milliseconds]] ,  result.[[Microseconds]] ,  result.[[Nanoseconds]] ) is false, throw a RangeError exception.

and change this part to be the exact text of

7.5.15 IsValidDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds )
from Temporal spec (probabaly except replacing the "to construct a Temporal.Duration" part to "to construct a DurationRecord"

@FrankYFTang
Copy link
Collaborator

Notice, with the PR , some test cases in
https://github.com/tc39/test262/blob/main/test/intl402/DurationFormat/prototype/format/precision-exact-mathematical-values.js

will be invalid and require changes to test the boundary condition and throwing/not throwing in the correct values

… for Temporal.Duration. This commit applies the same limits on valid values for Intl.DurationFormat DurationRecords.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion normative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants