-
Notifications
You must be signed in to change notification settings - Fork 537
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
Add time zone timestamp micros #1285
Add time zone timestamp micros #1285
Conversation
NaiveDateTime::from_timestamp_millis to reduce code duplication
Codecov Report
@@ Coverage Diff @@
## main #1285 +/- ##
==========================================
- Coverage 91.49% 91.48% -0.01%
==========================================
Files 35 35
Lines 16841 16860 +19
==========================================
+ Hits 15408 15425 +17
- Misses 1433 1435 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/offset/mod.rs
Outdated
} | ||
|
||
#[test] | ||
fn test_micros_example() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to copy the doctest here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I wasn't aware this gets tested automatically :)
src/offset/mod.rs
Outdated
/// | ||
/// assert_eq!(Utc.timestamp_micros_opt(1431648000000).unwrap().timestamp(), 1431648); | ||
/// ``` | ||
fn timestamp_micros_opt(&self, micros: i64) -> LocalResult<DateTime<Self>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this also use NaiveTime::from_timestamp_micros
?
@djc Do we want the _opt
suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested using NaiveTime::from_timestamp_micros
locally, it seems to work as expected.
Once we decide if you'll rather not have the _opt
suffix, I'll push my changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip the opt
suffix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe squash the last two commits?
Use NaiveDateTime::from_timestamp_micros instead of implementing the same logic again.
d9b3468
to
81708eb
Compare
I added
TimeZone::timestamp_micros_opt
and madeTimeZone::timestamp_millis_opt
useNaiveDateTime::from_timestamp_millis
to reduce logic duplication.resolves #1284