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.

Sign up for GitHub

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

Jump to bottom

Consistent verification of overflow options? #2220

Closed
Ms2ger opened this issue May 24, 2022 · 1 comment · Fixed by #2225
Closed

Consistent verification of overflow options? #2220

Ms2ger opened this issue May 24, 2022 · 1 comment · Fixed by #2225
Assignees
@ptomato
Labels
behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved

Comments

@Ms2ger
Copy link
Collaborator

Ms2ger commented May 24, 2022

E.g. PlainDate.from goes out of its way to call ToTemporalOverflow if the argument is already a PlainDate, but then it calls ToTemporalDate, which has fast paths for ZonedDateTime and PlainDateTime which don't call ToTemporalOverflow. Is this intentional?
@Ms2ger Ms2ger added behavior Relating to behavior defined in the proposal normative Would be a normative change to the proposal labels May 24, 2022
@ptomato
Copy link
Collaborator

ptomato commented May 24, 2022

This is probably not intentional. I think we need to check it in the fast paths, but not in the CalendarDateFromFields path since options is passed directly to dateFromFields.

ptomato added a commit that referenced this issue May 24, 2022
@ptomato
Normative: Consistently check overflow options
30c3f3f 
In the fast paths for Temporal objects in ToTemporalDate and
ToTemporalDateTime, the overflow option should be validated (even though
it isn't used.) We do this for property bags and also for the case where
we call Temporal.PlainDate.from(plainDate) and
Temporal.PlainDateTime.from(plainDateTime), so it should be consistent.

The status quo wasn't the result of any decision, it was an oversight.

Closes: #2220
@ptomato ptomato self-assigned this May 24, 2022
@ptomato ptomato added spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels May 24, 2022
@ptomato ptomato mentioned this issue May 24, 2022
Merged
ptomato added a commit that referenced this issue Jul 1, 2022
@ptomato
Normative: Consistently check overflow options
c8ee2bd 
In the fast paths for Temporal objects in ToTemporalDate and
ToTemporalDateTime, the overflow option should be validated (even though
it isn't used.) We do this for property bags and also for the case where
we call Temporal.PlainDate.from(plainDate) and
Temporal.PlainDateTime.from(plainDateTime), so it should be consistent.

The status quo wasn't the result of any decision, it was an oversight.

Closes: #2220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ptomato ptomato

Labels
behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Normative: Consistently check overflow options tc39/proposal-temporal
2 participants
@Ms2ger @ptomato