Simplify from_timestamp_millis, from_timestamp_micros

[pitdicker]

These methods are trivial when using div_euclid and rem_euclid. But the current somewhat complex implementation predates those methods in the standard library.

The panic in #934 related to this code did not reproduce, but I included it as a test case.

Closes #934.

[pitdicker]

Hmm, the keywords 'closes' and 'fixes' only work when the PR targets the default branch.

[esheppa]

Thanks for this @pitdicker - there are probably a few other places with similar logic that can be replaced as well given this works! (I'll look into the CI failures as they aren't caused by this PR).

#1033 should fix the CI failures here

[pitdicker]

@esheppa I didn't find other places yet, but your comment put me on track to find some simplification that allow removing the num-integer dependency in #1037.

[jtmoon79]

Should test cases be added that compare results of the "old way" and this "new way"?

I'd feel more comfortable with this if it proved it didn't break anything.

e.g.

/// compare old way of converting from millisconds using `_from_timestamp_unit`
/// to new way using `from_timestamp_millis`.
/// Prove they are equivalent.
#[test]
fn test_old__from_timestamp_unit_to_new_from_timestamp_millis {
    let cmp_millis = |x| assert_eq!(Self::_from_timestamp_unit(x, TimestampUnit::Millis), Self::from_timestamp_millis(x));
    cmp_millis(0);
    cmp_millis(1);
    cmp_millis(2);
    cmp_millis(998);
    cmp_millis(999);
    cmp_millis(1_000);
    cmp_millis(1_001);
    cmp_millis(1_002);
    cmp_millis(999_998);
    cmp_millis(999_999);
    cmp_millis(1_000_000);
    cmp_millis(1_000_001);
    cmp_millis(1_000_002);
    cmp_millis(1682489137231); // Wednesday, April 26, 2023 6:05:37.231Z
    cmp_millis(MAX - 2);
    cmp_millis(MAX - 1);
    cmp_millis(MAX);
}

/// OLD METHOD; DO NOT USE. ONLY KEPT AROUND TO PROVE `from_timestamp_millis` AND `from_timestamp_micros` PERFORM EQUIVALENTLY.
/// Creates a new [NaiveDateTime] from milliseconds or microseconds since the UNIX epoch.
///
/// This is a private function used by [from_timestamp_millis] and [from_timestamp_micros].
fn _from_timestamp_unit(...) {

Same test for the microseconds version.
...though, I wonder if there is already such a test case but at a higher level (i.e. input a Epoch timestamp, assert output datetime result)? 👀

[pitdicker]

I'll look at what tests are covering this and otherwise add one 👍

[pitdicker]

I wonder what happened to my commit with a test for #934, must have lost it somewhere...

The existing test for these methods are naive::datetime::NaiveDateTime::from_timestamp_millis and naive::datetime::NaiveDateTime::from_timestamp_micros, and the doctests. I extended them with your test cases.

/// OLD METHOD; DO NOT USE. ONLY KEPT AROUND TO PROVE `from_timestamp_millis` AND `from_timestamp_micros` PERFORM EQUIVALENTLY.
/// Creates a new [NaiveDateTime] from milliseconds or microseconds since the UNIX epoch.
///
/// This is a private function used by [from_timestamp_millis] and [from_timestamp_micros].
fn _from_timestamp_unit(...) {

Honestly, I don't feel like what these methods warrants more complex tests. Adding them is no problem, it just feels out of place.

[jtmoon79]

I wonder what happened to my commit with a test for #934, must have lost it somewhere...

The existing test for these methods are naive::datetime::NaiveDateTime::from_timestamp_millis and naive::datetime::NaiveDateTime::from_timestamp_micros, and the doctests. I extended them with your test cases.

/// OLD METHOD; DO NOT USE. ONLY KEPT AROUND TO PROVE `from_timestamp_millis` AND `from_timestamp_micros` PERFORM EQUIVALENTLY.
/// Creates a new [NaiveDateTime] from milliseconds or microseconds since the UNIX epoch.
///
/// This is a private function used by [from_timestamp_millis] and [from_timestamp_micros].
fn _from_timestamp_unit(...) {

Honestly, I don't feel like what these methods warrants more complex tests. Adding them is no problem, it just feels out of place.

Yeah, my suggestion get's kinda' ugly.

What about this:

• commit A: add test cases for current code

  #[test]
  fn test_from_timestamp_millis() {
      assert_eq!(Self::from_timestamp_millis(0), Some(ymdhms(1970, 1, 1, 0, 0, 0));
      assert_eq!(Self::from_timestamp_millis(1), Some(...));
      assert_eq!(Self::from_timestamp_millis(2), Some(...));
      assert_eq!(Self::from_timestamp_millis(998), Some(...));
      assert_eq!(Self::from_timestamp_millis(999), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_000), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_001), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_002), Some(...));
      assert_eq!(Self::from_timestamp_millis(999_998), Some(...));
      assert_eq!(Self::from_timestamp_millis(999_999), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_000_000), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_000_001), Some(...));
      assert_eq!(Self::from_timestamp_millis(1_000_002), Some(...));
      assert_eq!(Self::from_timestamp_millis(1682489137231), Some(...)); // Wednesday, April 26, 2023 6:05:37.231Z
      // ...
      // more asserts, get creative!
      // ...
      assert_eq!(Self::from_timestamp_millis(MAX - 2), Some(...));
      assert_eq!(Self::from_timestamp_millis(MAX - 1), Some(...));
      assert_eq!(Self::from_timestamp_millis(MAX), Some(...));
      // None cases
      assert_eq!(Self::from_timestamp_millis(???), None);
  }
  
  #[test]
  fn test_from_timestamp_micros() {
     // asserts!
     // ...
     // assert_eq!(Self::from_timestamp_micros(x), y)
     // ...
  }

• commit B: make changes proposed in this PR, prior-added test cases should not affected

[pitdicker]

😄 Yes, I added the test in a commit before the changes so it was easy to test the old and new implementation. a419c4b

more asserts, get creative!

Probably dates before 1970 are interesting to test.

[jtmoon79]

😄 Yes, I added the test in a commit before the changes so it was easy to test the old and new implementation. a419c4b

more asserts, get creative!

Probably dates before 1970 are interesting to test.

Oh for some reason I had entirely overlooked that commit (and tests). But yeah, a few more wouldn't hurt; up to you.

[djc]

Going to merge this with the existing test coverage for now, thanks!