Skip to content

Conversation

jedel1043
Copy link
Member

This PR introduces an Epoch struct on the utils module, which unifies all our APIs into a single utility type.

Marking as draft for now since I'd like to test against Boa first, but it's reviewable.

@jedel1043 jedel1043 added the C-internal Internal library improvements label Feb 25, 2025
@jedel1043 jedel1043 requested a review from nekevss February 25, 2025 07:14
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.

Really like the work to clean this up. I had a thought when I saw the new Epoch struct. Let me know what you think.


pub(crate) use neri_schneider::epoch_days_from_gregorian_date;
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) struct Epoch {
Copy link
Member

Choose a reason for hiding this comment

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

thought: Potentially rename to EpochMilliseconds and upgrade to pub.

We have an EpochNanoseconds type that's already public, and I was already debating whether to have a public EpochMillisecond for Instant::from_milliseconds and use EpochNanoseconds in Instant::from_nanoseconds. This could then result in pulling a majority of the EpochMilliseconds type out of utils into something like epoch_primitives, epoch, or something. I'm not really sure about the module name. I'm not in love with where EpochNanoseconds is exported from currently either, which I think is temporal_rs::time. Heck, temporal_rs::epoch probably makes even more sense than time.

Doing this then leaves whatever is left in utils as pure utility functions.

//! Utility date and time equations for Temporal
#![allow(
unused,
reason = "prefer to have unused methods instead of having to gate everything behind features"
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably a better approach.

#[inline]
#[must_use]
pub fn padded_iso_year_string(&self) -> String {
pad_iso_year(self.iso.year)
Copy link
Member

Choose a reason for hiding this comment

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

I think this can actually be removed. We're not using this in Boa and to_ixdtf_string supersedes it. For some reason, I didn't notice this was still part of the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-internal Internal library improvements
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants