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

Proposal to limit rounding increments for hour, month, week, and day #2458

Closed
ptomato opened this issue Jan 6, 2023 · 2 comments · Fixed by #2480
Closed

Proposal to limit rounding increments for hour, month, week, and day #2458

ptomato opened this issue Jan 6, 2023 · 2 comments · Fixed by #2480
Assignees
Labels
feedback normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Jan 6, 2023

Feedback from @anba in #1502:

Unlimited increment for "hour", "month", "week", and "day" has some issues:

  1. Too large increments don't seem useful for the user, because the other input is always restricted to a certain range. For example the number of days between two dates is restricted to 2×10⁸ days by definition. When the rounding increment is then set to, let's say 10⁹, the rounding result will always either be 0 or 10⁹, depending on the rounding mode. (Or in other cases TypeErrors, because some intermediate results can't be represented.)
  2. And unlimited increments make harder to implement this, because it's not possible to use fixed size integer types.

Therefore I'd like to propose to limit the maximum rounding increment for "hour", "month", "week", and "day" to reasonable limits.

(My apologies for not noticing and pulling this out of the checklist earlier.)

Example of what's being referred to in (1):

Temporal.PlainDate.from('2023-01-05').until('2023-01-06', {roundingIncrement: 1e9})
  // => PT0S
Temporal.PlainDate.from('2023-01-05').until('2023-01-06', {roundingIncrement: 1e9, roundingMode: 'ceil'})
  // => P1000000000D
@ptomato ptomato added spec-text Specification text involved meeting-agenda feedback normative Would be a normative change to the proposal labels Jan 6, 2023
@ptomato ptomato added this to the Stage "3.5" milestone Jan 6, 2023
@ptomato
Copy link
Collaborator Author

ptomato commented Jan 6, 2023

Personally, I'm not in favour of this; one person's reasonable limit is another's unreasonable restriction.

Would the storage issue (2) be able to be solved by using a double, since the increment is guaranteed to originate as a JS Number?

@justingrant
Copy link
Collaborator

justingrant commented Jan 12, 2023

Meeting 2023-01-12: we'll limit all currently-unlimited increments to 109 (or lower) so they can fit in a 32-bit integer. We'll use 109 instead of 231-1 or 232-1 because 109 will be clearer in end-user-facing documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants