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

Update boa_temporal Time Zone design #3543

Merged
merged 1 commit into from Jan 5, 2024
Merged

Update boa_temporal Time Zone design #3543

merged 1 commit into from Jan 5, 2024

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Dec 30, 2023

The updates on this branch is to bring the time zone implementation in line with the design from #3522 and related to #1804.

It changes the following:

  • Changes TimeZoneSlot to the below
pub enum TimeZoneSlot<Z: TzProtocol> {
    /// A native `TimeZone` representation.
    Tz(TimeZone),
    /// A Custom `TimeZone` that implements the `TzProtocol`.
    Protocol(Z),
}
  • Adds the custom time zone object (with a Trace impl).

Notably, as further context on the below, Time Zones right now is mostly the structure/scaffolding with limited functionality. We'll need to figure out a good way to handle tz system calls and IANA mapping (whether that be using some icu4x provider or baking something ourselves based from the IANA tz database) to really build this out functionality wise and test it, at least imo. If anyone has any thoughts, let me know 😄

Copy link

Test262 conformance changes

Test result main count PR count difference
Total 95,960 95,960 0
Passed 76,534 76,534 0
Ignored 18,477 18,477 0
Failed 949 949 0
Panics 0 0 0
Conformance 79.76% 79.76% 0.00%

Copy link

codecov bot commented Dec 30, 2023

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (a20a61a) 47.42% compared to head (c3caed9) 47.38%.

Files Patch % Lines
...e/engine/src/builtins/temporal/time_zone/custom.rs 0.00% 25 Missing ⚠️
...ngine/src/builtins/temporal/zoned_date_time/mod.rs 0.00% 7 Missing ⚠️
core/engine/src/builtins/temporal/time_zone/mod.rs 0.00% 6 Missing ⚠️
core/temporal/src/components/tz.rs 16.66% 5 Missing ⚠️
core/temporal/src/components/zoneddatetime.rs 33.33% 2 Missing ⚠️
core/temporal/src/components/duration.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
- Coverage   47.42%   47.38%   -0.04%     
==========================================
  Files         470      471       +1     
  Lines       45690    45727      +37     
==========================================
  Hits        21667    21667              
- Misses      24023    24060      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nekevss nekevss requested a review from a team December 30, 2023 00:45
@nekevss nekevss added builtins PRs and Issues related to builtins/intrinsics waiting-on-review Waiting on reviews from the maintainers labels Dec 30, 2023
@nekevss nekevss added this to the v0.18.0 milestone Dec 30, 2023
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! :)

@nekevss nekevss requested a review from a team December 31, 2023 23:48
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Good progress! I have a suggestion for the next ergonomics change, but this can be merged as is.

/// The Time Zone Protocol that must be implemented for time zones.
pub trait TzProtocol: TzProtocolClone {
pub trait TzProtocol: Clone {
Copy link
Member

Choose a reason for hiding this comment

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

Thought: We should probably add an associated error type type Error: From<TemporalError> so that we can preserve the full engine error. Would also have to be added to CalendarProtocol.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. As the change can be done on both traits. I'll update that on another PR for both traits.

@nekevss nekevss added this pull request to the merge queue Jan 5, 2024
Merged via the queue into main with commit 0cb17cf Jan 5, 2024
14 checks passed
@jedel1043 jedel1043 deleted the temporal-tz-update branch January 5, 2024 19:46
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 waiting-on-review Waiting on reviews from the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants