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

Remove Temporal.Calendar and Temporal.TimeZone #3890

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Jul 4, 2024

We will probably miss some changes when the spec is patched with the removals, but we can do a pass through temporal when that time comes.

Also checked all the regressions and they're failing because they try to access the removed Temporal.Calendar in some way or another, so that's fine.

@jedel1043 jedel1043 added the builtins PRs and Issues related to builtins/intrinsics label Jul 4, 2024
@jedel1043 jedel1043 added this to the v0.19.0 milestone Jul 4, 2024
@jedel1043 jedel1043 requested a review from a team July 4, 2024 03:05
@jedel1043 jedel1043 force-pushed the remove-cal-tz branch 2 times, most recently from d0b8a3b to d654ce6 Compare July 4, 2024 03:17
Copy link

github-actions bot commented Jul 4, 2024

Test262 conformance changes

Test result main count PR count difference
Total 50,213 48,212 -2,001
Passed 42,981 42,611 -370
Ignored 1,411 1,413 +2
Failed 5,821 4,188 -1,633
Panics 0 0 0
Conformance 85.60% 88.38% +2.79%
Fixed tests (9):
test/built-ins/Temporal/PlainYearMonth/calendar-wrong-type.js (previously Failed)
test/built-ins/Temporal/ZonedDateTime/from/argument-propertybag-calendar-number.js (previously Failed)
test/built-ins/Temporal/PlainDateTime/calendar-wrong-type.js (previously Failed)
test/built-ins/Temporal/PlainDateTime/calendar-string.js (previously Failed)
test/built-ins/Temporal/PlainDateTime/constructor-full.js (previously Failed)
test/built-ins/Temporal/PlainDate/calendar-undefined.js (previously Failed)
test/built-ins/Temporal/PlainDate/calendar-wrong-type.js (previously Failed)
test/built-ins/Temporal/PlainDate/basic.js (previously Failed)
test/built-ins/Temporal/PlainDate/calendar-string.js (previously Failed)

@nekevss
Copy link
Member

nekevss commented Jul 4, 2024

Hmm, would it be worth while doing this in an intermediate stage of removing the custom functionality and waiting for the PR on the specification for the removal of the entire builtin?

@jedel1043
Copy link
Member Author

Hmm, would it be worth while doing this in an intermediate stage of removing the custom functionality and waiting for the PR on the specification for the removal of the entire builtin?

That was the plan, but almost everything on the Temporal.Calendar and Temporal.TimeZone builtins depended on things that were removed in temporal_rs, so I had to remove all methods from those.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good, I'd be ok with merging it now.

@jedel1043 jedel1043 requested a review from a team July 4, 2024 21:08
@nekevss
Copy link
Member

nekevss commented Jul 4, 2024

The custom functionality was removed from both, but weren't the core methods kept for calendars and time zones?

@jedel1043
Copy link
Member Author

The custom functionality was removed from both, but weren't the core methods kept for calendars and time zones?

On the spec side? Nope, the plan is to delete both builtins and only be able to access the calendar/timezone by IANA ID or ca value, respectively.

On the temporal_rs side? Yes, we have the core methods still, it was just easier to remove everything instead of having to patch custom versions of those methods with all the custom calendar functionality removed.

@nekevss
Copy link
Member

nekevss commented Jul 5, 2024

That makes sense. I'm just a little hesitant to preemptively remove the builtins based on the removal issues and plans without confirming with the champions the most recent consensus and timeline for the updates to the specification. Because the builtins are currently in the spec and test suite, and there is a history of things changing (even though that seems to be slowing down).

@jedel1043
Copy link
Member Author

I mean, there's already a PR to remove those builtins from the tests, and it has been approved already, so...
tc39/test262#4119

Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Huh, they hadn't updated the proposal repository or the specification for temporal. Given that the PR to remove the tests has been merged, then let's move forward with merging this.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me! :)

Just need to add the "source-phase-imports" feature to the known features of the boa_tester, that should fix the CI.

@jedel1043 jedel1043 added this pull request to the merge queue Jul 5, 2024
Merged via the queue into main with commit 8e76836 Jul 5, 2024
13 checks passed
@jedel1043 jedel1043 deleted the remove-cal-tz branch July 5, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants