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

Temporal.TimeZone.prototype.getOffsetStringFor: Move CreateTimeZoneMethodsRecord after ToTemporalInstant? #2775

Closed
anba opened this issue Feb 20, 2024 · 2 comments · Fixed by #2787
Assignees

Comments

@anba
Copy link
Contributor

anba commented Feb 20, 2024

Temporal.TimeZone.prototype.getOffsetStringFor calls CreateTimeZoneMethodsRecord and then ToTemporalInstant, whereas Temporal.TimeZone.prototype.getPlainDateTimeFor and Temporal.TimeZone.prototype.getInstantFor first call ToTemporalInstant and then CreateTimeZoneMethodsRecord.

@ptomato
Copy link
Collaborator

ptomato commented Feb 21, 2024

I'm fine with this if it wouldn't be considered a normative change, but an after-the-fact review comment on #2519. (The inconsistency was obviously not intended, and the reference JS code doesn't have the inconsistency.)

@anba
Copy link
Contributor Author

anba commented Feb 23, 2024

I'm fine with this if it wouldn't be considered a normative change, but an after-the-fact review comment on #2519. (

I think it's reasonable to consider this as an after-the-fact review comment.

@ptomato ptomato self-assigned this Feb 24, 2024
ptomato added a commit that referenced this issue Feb 24, 2024
This was an omission in #2519, a recently-merged normative change, that
Anba spotted after the fact. Possibly a rebase error.

Makes getOffsetStringFor consistent with getPlainDateTimeFor and
getInstantFor. The reference code was already correct here.

Closes: #2775
Ms2ger pushed a commit that referenced this issue Feb 26, 2024
This was an omission in #2519, a recently-merged normative change, that
Anba spotted after the fact. Possibly a rebase error.

Makes getOffsetStringFor consistent with getPlainDateTimeFor and
getInstantFor. The reference code was already correct here.

Closes: #2775
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 a pull request may close this issue.

2 participants