Utilize EpochNanoseconds #261
Conversation
…temporal into impl-to-zoned-date-time
|
Hm. All the previous commits from my previous branch also got added. Will need to check into that as well. |
src/builtins/core/instant.rs
Outdated
| } | ||
|
|
||
| /// Creates a new `Instant` from the provided Epoch millisecond value. | ||
| pub fn from_epoch_milliseconds(epoch_milliseconds: i64) -> TemporalResult<Self> { | ||
| pub fn from_epoch_milliseconds(epoch_milliseconds: EpochNanoseconds) -> TemporalResult<Self> { |
There was a problem hiding this comment.
Hmmmmm, I believe this isn't entirely correct since we are dealing with milliseconds vs. nanoseconds. This should probably be left as is or a new EpochMilliseconds primitive type could be introduced with an internal representation of i64.
src/builtins/core/timezone.rs
Outdated
| let nanos = self.get_offset_nanos_for(instant.as_i128(), provider)?; | ||
| IsoDateTime::from_epoch_nanos(instant.epoch_nanoseconds(), nanos.to_i64().unwrap_or(0)) | ||
| let nanos = self.get_offset_nanos_for(instant.epoch_nanoseconds(), provider)?; | ||
| IsoDateTime::from_epoch_nanos(instant.epoch_nanoseconds(), nanos.as_i128().to_i64().unwrap_or(0)) |
There was a problem hiding this comment.
thought: I think to_i64 could be implemented directly on EpochNanoseconds as a method. We're already doing the bound check, so it could be a nice addition from an ergonomics perspective.
|
Not so well with git so not sure to do with that mistake. Updated from your feedback. Thanks! Added the helper method. Altered a test, but should have the same functionality? |
There was a problem hiding this comment.
Hey! Sorry for the delayed review on this. To be honest, I'm not entirely sure what to make of this PR (and not because there is anything necessarily wrong with it) at least until we have built out the provider crate a bit more thoroughly. I'm fairly convinced that using EpochNanoseconds more thoroughly is a good thing, but I'm not entirely sure to what degree currently.
I think this may be soft blocked until we are a bit further into the process of setting up the provider crate and have a better idea of what should live where.
I left some comments explaining my general thoughts.
| @@ -43,6 +43,10 @@ impl EpochNanoseconds { | |||
| pub fn as_i128(&self) -> i128 { | |||
| self.0 | |||
| } | |||
|
|
|||
| pub fn as_i64(&self) -> i64 { | |||
There was a problem hiding this comment.
Hmmmmm, I think this needs to be an Option<i64>. It's be a while so I could be misremembering, but I believe the max nanoseconds exceeds an i64.
| @@ -66,14 +66,14 @@ pub trait TimeZoneProvider { | |||
| fn get_named_tz_offset_nanoseconds( | |||
| &self, | |||
| identifier: &str, | |||
| epoch_nanoseconds: i128, | |||
| epoch_nanoseconds: EpochNanoseconds, | |||
There was a problem hiding this comment.
thought: I'm a little concerned about adding the EpochNanoseconds to the TimeZoneProvider trait.
Mostly because the current plan is to move TimeZoneProvider out of temporal_rs. So tying in a temporal_rs type to the TimeZoneProvider's interface creates a small problem of what should be done with EpochNanoseconds when the trait is moved. 😕
Solves #119
Started fresh up the code, one test failing in the instant class still.
Im a commits behind, but wanted to merge when the code was working locally first.
Some things i'm unsure about:
Thanks!
Let me know if you have any quick comments