NaiveDateTime::from_timestamp_millis(_opt) #818

Pscheidl · 2022-09-11T19:07:52Z

Construct NaiveDateTime from millis since epoch.

Addresses #430.

djc · 2022-09-11T21:10:37Z

Despite the consistency argument I'm inclined to ditch the _opt() suffix from the fallible method and leave potential unwrapping to the caller (that is -- not add the panicking variant at all). This seems clearly better to me and the way of the future. @esheppa thoughts?

Pscheidl · 2022-09-12T07:02:04Z

Despite the consistency argument I'm inclined to ditch the _opt() suffix from the fallible method and leave potential unwrapping to the caller (that is -- not add the panicking variant at all). This seems clearly better to me and the way of the future. @esheppa thoughts?

Done (we can always revert to the original).

Construct NaiveDateTime from millis since epoch

djc

Thanks for working on this! LGTM modulo some documentation nits.

djc · 2022-09-12T13:35:02Z

src/naive/datetime/mod.rs

+    /// from the number of milliseconds
+    /// since the midnight UTC on January 1, 1970 (aka "UNIX timestamp").
+    ///
+    /// Returns `None` on the out-of-range number of milliseconds and/or invalid nanosecond.


In this case, there's no "invalid nanosecond", right? Also I think "the" -> "an" would read better?

No, there are no invalid nanos in this case. Changed.

djc · 2022-09-12T13:36:01Z

src/naive/datetime/mod.rs

@@ -127,6 +127,28 @@ impl NaiveDateTime {
        datetime.expect("invalid or out-of-range datetime")
    }

+    /// Makes a new `NaiveDateTime` corresponding to a UTC date and time,


You probably copy-pasted this from one of the other methods, but we're trying to make the first line of docstrings self-contained sentence fragments. After that we should have an empty line, then a more detailed description. Would you mind tweaking the docs for this new method at least?

I think the whole doc can be simplified. Gave it a try. Please have a look.

djc

Seems like an improvement, some remaining nits.

djc · 2022-09-12T14:33:02Z

src/naive/datetime/mod.rs

    ///
-    /// Returns `None` on the out-of-range number of milliseconds and/or invalid nanosecond.
+    ///  Returns `None` on an out-of-range number of milliseconds.


Should ditch the prefix space here.

djc · 2022-09-12T14:33:46Z

src/naive/datetime/mod.rs

-    /// Makes a new `NaiveDateTime` corresponding to a UTC date and time,
-    /// from the number of milliseconds
-    /// since the midnight UTC on January 1, 1970 (aka "UNIX timestamp").
+    /// Creates a new [NaiveDateTime] from milliseconds since UNIX Epoch.


Both here and in the next line, I think prefixing "UNIX" with "the" ("the UNIX epoch") would read better.

Sure, done,

esheppa · 2022-09-13T11:24:32Z

src/naive/datetime/mod.rs

+    #[inline]
+    pub fn from_timestamp_millis(millis: i64) -> Option<NaiveDateTime> {
+        let secs = millis / 1000;
+        let nsecs = (millis % 1000) as u32 * 1_000_000;


this looks problematic as the input millis could be negative, and will still be negative after taking the modulo (playground)

One option would be to have two cases, depending on whether millis is positive or negative, similar to the logic used here

The other option is just:
NaiveDate::from_ymd(1970,1, 1).and_hms(0, 0, 0) + TimeDelta::from_millis(millis)

Also it would be preferable to use TryFrom instead of as

Awesome, let's deal with that. I'm super busy ATM, please give me by the end of the week :)

I don't think we should go through TimeDelta here, which adds a bunch of overhead and feels like a layering violation. Would be better to do a minimal calculation and still defer to NaiveDateTime::from_timestamp_opt().

Good call RE. avoiding TimeDelta especially as from_millis may well get deprecated anyway

Done. The nanos calculation is split, as when the timestamp in millis is negative, an additional abs() op is done.

esheppa · 2022-09-13T11:26:51Z

Thanks for the PR @Pscheidl - this will be a useful helper function to have, and the docs look good - just noted a possible issue in the implementation when the input timestamp is negative

src/naive/datetime/mod.rs

Pscheidl · 2022-09-18T06:15:02Z

All comments addressed.

esheppa · 2022-09-18T07:01:38Z

src/naive/datetime/mod.rs

+
+        if millis < 0 {
+            let nsecs = (millis % 1000).abs() as u32 * NANOS_IN_MILLISECOND;
+            NaiveDateTime::from_timestamp_opt(secs - 1, NANOS_IN_SECOND - nsecs)


Thanks for the impl that handles negative millis. If possible could you subtract with checked_sub here to avoid potential overflow

esheppa · 2022-09-18T07:03:54Z

src/naive/datetime/mod.rs

+        let secs = millis / 1000;
+
+        if millis < 0 {
+            let nsecs = (millis % 1000).abs() as u32 * NANOS_IN_MILLISECOND;


Do you mind switching as for u32::try_from(expr).ok()?

esheppa · 2022-09-18T07:04:05Z

src/naive/datetime/mod.rs

+            let nsecs = (millis % 1000).abs() as u32 * NANOS_IN_MILLISECOND;
+            NaiveDateTime::from_timestamp_opt(secs - 1, NANOS_IN_SECOND - nsecs)
+        } else {
+            let nsecs = (millis % 1000) as u32 * NANOS_IN_MILLISECOND;


Do you mind switching as for u32::try_from(expr).ok()?

esheppa · 2022-09-18T07:06:29Z

Thanks @Pscheidl, the new negative millis handling looks good- I've just requested a few more small changes

…on err

Pscheidl · 2022-09-18T15:47:46Z

Awesome @esheppa , thank you. All comments addressed.

esheppa

Thanks for those changes @Pscheidl

Pscheidl · 2022-09-19T07:20:36Z

Learned a lot about the crate, thank you for the guidance.

I can't hit the merge button, as I don't have write access.

djc · 2022-09-19T09:31:52Z

Oops, should have squashed this instead of rebasing. :(

djc · 2022-09-19T09:50:49Z

Backport to 0.4.x is in #823, which also includes some deduplication of the logic.

Pscheidl force-pushed the 430 branch from 7bfa8fb to 940f2fe Compare September 12, 2022 07:01

NaiveDateTime::from_timestamp_millis(_opt)

341ed5a

Construct NaiveDateTime from millis since epoch

Pscheidl force-pushed the 430 branch from 940f2fe to 341ed5a Compare September 12, 2022 12:53

djc reviewed Sep 12, 2022

View reviewed changes

docs: Simplify documentation for NaiveDateTime::from_timestamp_millis

80885d8

Pscheidl force-pushed the 430 branch from e4bf665 to 80885d8 Compare September 12, 2022 14:23

djc reviewed Sep 12, 2022

View reviewed changes

docs: prefix UNIX with the, remove whitespaces

63b26bb

djc approved these changes Sep 13, 2022

View reviewed changes

djc requested a review from esheppa September 13, 2022 09:47

esheppa reviewed Sep 13, 2022

View reviewed changes

esheppa requested changes Sep 13, 2022

View reviewed changes

src/naive/datetime/mod.rs Outdated Show resolved Hide resolved

fix: Negative UNIX timestamps accepted by from_timestamp_millis

b175f9c

Pscheidl force-pushed the 430 branch from 559ade1 to b175f9c Compare September 14, 2022 20:06

esheppa requested changes Sep 18, 2022

View reviewed changes

refactor: Use checked substitution & convert u32 conversiont to None …

de12842

…on err

esheppa approved these changes Sep 18, 2022

View reviewed changes

djc approved these changes Sep 19, 2022

View reviewed changes

djc merged commit 40800ce into chronotope:main Sep 19, 2022

djc mentioned this pull request Oct 18, 2022

0.4.23 release planning #850

Closed

esheppa mentioned this pull request Nov 3, 2022

request to simplify parsing unix millis #862

Closed

pitdicker mentioned this pull request Jun 7, 2023

NaiveDateTime::from_millis #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NaiveDateTime::from_timestamp_millis(_opt) #818

NaiveDateTime::from_timestamp_millis(_opt) #818

Pscheidl commented Sep 11, 2022 •

edited

djc commented Sep 11, 2022 •

edited

Pscheidl commented Sep 12, 2022

djc left a comment

djc Sep 12, 2022

Pscheidl Sep 12, 2022

djc Sep 12, 2022

Pscheidl Sep 12, 2022

djc left a comment

djc Sep 12, 2022

Pscheidl Sep 12, 2022

djc Sep 12, 2022

Pscheidl Sep 12, 2022

esheppa Sep 13, 2022

Pscheidl Sep 14, 2022

djc Sep 14, 2022

esheppa Sep 14, 2022

Pscheidl Sep 14, 2022

esheppa commented Sep 13, 2022

Pscheidl commented Sep 18, 2022

esheppa Sep 18, 2022

Pscheidl Sep 18, 2022

esheppa Sep 18, 2022

Pscheidl Sep 18, 2022

esheppa Sep 18, 2022

Pscheidl Sep 18, 2022

esheppa commented Sep 18, 2022

Pscheidl commented Sep 18, 2022

esheppa left a comment

Pscheidl commented Sep 19, 2022

djc commented Sep 19, 2022

djc commented Sep 19, 2022

NaiveDateTime::from_timestamp_millis(_opt) #818

NaiveDateTime::from_timestamp_millis(_opt) #818

Conversation

Pscheidl commented Sep 11, 2022 • edited

djc commented Sep 11, 2022 • edited

Pscheidl commented Sep 12, 2022

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esheppa commented Sep 13, 2022

Pscheidl commented Sep 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esheppa commented Sep 18, 2022

Pscheidl commented Sep 18, 2022

esheppa left a comment

Choose a reason for hiding this comment

Pscheidl commented Sep 19, 2022

djc commented Sep 19, 2022

djc commented Sep 19, 2022

Pscheidl commented Sep 11, 2022 •

edited

djc commented Sep 11, 2022 •

edited