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

Implement PartialZonedDateTime and from_partial and from_str for ZonedDateTime #115

Merged
merged 7 commits into from
Dec 7, 2024

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Nov 26, 2024

Posting this early as a draft as the logic was pretty dense to work through.

This PR still needs some baseline tests added for the methods before it's ready to not be a draft.

Update:

Tests have been added! There's also a bug fix in tzdb.rs.

Overall, this implements the core logic in temporal_rs for supporting ToTemporalZonedDateTime and should unblock implementing Temporal.ZonedDateTime.from in Boa.

Overview:

  • Adds PartialZonedDateTime
  • Adds from_partial and from_partial_with_provider
  • Adds from_str and from_str_with_provider
  • Adds additional tests for the above
  • Fix bug in tzdb.rs get_local_record
  • Implements some related abstract ops and moves IsoDateTime::balance away from using f64.

@nekevss nekevss marked this pull request as ready for review November 26, 2024 23:52
@nekevss nekevss added the C-enhancement New feature or request label Nov 27, 2024
pub trait TzProvider {
fn check_identifier(&self, identifier: &str) -> bool;

fn get_named_tz_epoch_nanoseconds(
&self,
identifier: &str,
iso_datetime: IsoDateTime,
) -> TemporalResult<Vec<i128>>;
) -> TemporalResult<Vec<EpochNanoseconds>>;
Copy link
Member Author

@nekevss nekevss Dec 5, 2024

Choose a reason for hiding this comment

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

I noticed when rebasing this PR that EpochNanoseconds can (and probably should) be used in more places throughout temporal_rs then I had initially thought. I added some of that work to this PR due to it somewhat being in the scope of work here. For the rest though, I'm thinking it would be best to open up an issue for it and continue looking into using EpochNanoseconds in more places for a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed! Could be a nice first issue for a future contributor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened an issue for it 😄

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!

millisecond: f64,
microsecond: f64,
nanosecond: f64,
hour: i64,
Copy link
Member

Choose a reason for hiding this comment

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

You should update the comment above that says an f64 is needed here....

#[cfg(feature = "experimental")]
use crate::components::tz::TZ_PROVIDER;
#[cfg(feature = "experimental")]
use std::ops::Deref;

/// A struct representing a partial `ZonedDateTime`.
pub struct PartialZonedDateTime {
/// The `PartialDate` portion of a `PartialDateTime`
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment supposed to say "The PartialDate portion of a PartialZonedDateTimeor the PartialDateTime is correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opps, forgot to type Zoned 😂

@nekevss nekevss merged commit e153634 into main Dec 7, 2024
5 checks passed
nekevss added a commit that referenced this pull request Dec 7, 2024
This PR is dependent on #115 and will need a rebase once that PR is
merged.

This continues to build out methods that `ZonedDateTime` needs for
accessor methods.

Overall the features implemented are:

  - All calendar accessor methods
  - An implementation of `HoursInDay` method
@nekevss nekevss deleted the impl-partial-zdt branch December 9, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants