Conversation
cd8650e to
0615cd2
Compare
0615cd2 to
91f069b
Compare
nekevss
left a comment
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Yeah, probably a better approach.
| #[inline] | ||
| #[must_use] | ||
| pub fn padded_iso_year_string(&self) -> String { | ||
| pad_iso_year(self.iso.year) |
There was a problem hiding this comment.
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.
This PR introduces an
Epochstruct 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.