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

Next batch of editorial issues #2922

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Next batch of editorial issues #2922

merged 8 commits into from
Aug 8, 2024

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jul 24, 2024

Best reviewed commit by commit. Explanations in each commit message. Closes several issues opened by @anba.

ptomato added 7 commits July 23, 2024 16:44
This is one step towards breaking the annoying circular import between
calendar.mjs and ecmascript.mjs.
This observable call occurred while building an error message. It
shouldn't be there according to the spec.
roundedFractionalDays/roundedDaysRemainder isn't needed, and would be more
of a hassle for implementations to calculate anyway.

Closes: #2889
There is already a test262 case that tests throwing here:
test/built-ins/Temporal/ZonedDateTime/prototype/until/roundingincrement-addition-out-of-range.js
Thanks to Anba for spotting this.

Closes: #2891
Thanks to Anba for spotting this.
These weren't used anymore after removing Now.plainDate/plainDateTime/
zonedDateTime.
Thanks to Anba for spotting this.
Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

Looks great, thanks!

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

These arguments aren't optional in the corresponding spec text AOs either.
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.

4 participants