Skip to content
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 DateTimeUtils conversion methods #260

Merged
merged 3 commits into from Sep 6, 2019

Conversation

@gbrodman
Copy link
Collaborator

commented Sep 5, 2019

This change is Reviewable

@gbrodman gbrodman requested a review from CydeWeys Sep 5, 2019

@googlebot googlebot added the cla: yes label Sep 5, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @CydeWeys and @gbrodman)


util/src/main/java/google/registry/util/DateTimeUtils.java, line 108 at r1 (raw file):

    return new DateTime(
        zonedDateTime.toInstant().toEpochMilli(),
        DateTimeZone.forID(zonedDateTime.getZone().getId()));

It sounds like forID can throw exceptions? See here: https://stackoverflow.com/a/37335420

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

    assertThat(zonedDateTime.getZone().getId()).isEqualTo("UTC");
  }

I'm curious to see a test on a time zone of "Z" per the linked Stackoverflow article above.

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)


util/src/main/java/google/registry/util/DateTimeUtils.java, line 108 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It sounds like forID can throw exceptions? See here: https://stackoverflow.com/a/37335420

yeah, this is correct and should be modified


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm curious to see a test on a time zone of "Z" per the linked Stackoverflow article above.

yeah, this is correct and should be modified

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

Previously, gbrodman wrote…

yeah, this is correct and should be modified

Can you add this test?

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can you add this test?

I did -- that's ZoneOffset.UTC on line 122.

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

Previously, gbrodman wrote…

I did -- that's ZoneOffset.UTC on line 122.

Can you add a test that uses a Joda DateTime that comes from something like DateTime.parse("2015-10-13T11:22:33.168Z") ?

We use that explicit Z notation all over our codebase. I get that ZoneOffset.UTC should be that, but it'd be even more clear to see the Z written out in text.

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @gbrodman)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 106 at r2 (raw file):

  @Test
  public void testSuccess_toZonedDateTime() {
    DateTime dateTime = DateTime.now(DateTimeZone.UTC);

It's better to use a specific time rather than just whatever now happens to be. Tests should be testing the same thing each time. We don't want a test that works now and then unexpectedly starts failing come leap day or something.

Separately, maybe a leap day test would be nice? :D

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @CydeWeys)


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 110 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can you add a test that uses a Joda DateTime that comes from something like DateTime.parse("2015-10-13T11:22:33.168Z") ?

We use that explicit Z notation all over our codebase. I get that ZoneOffset.UTC should be that, but it'd be even more clear to see the Z written out in text.

SGTM, I changed them all to be from strings.


util/src/test/java/google/registry/util/DateTimeUtilsTest.java, line 106 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It's better to use a specific time rather than just whatever now happens to be. Tests should be testing the same thing each time. We don't want a test that works now and then unexpectedly starts failing come leap day or something.

Separately, maybe a leap day test would be nice? :D

Done.

@CydeWeys
Copy link
Member

left a comment

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved

@gbrodman gbrodman merged commit 729b695 into google:master Sep 6, 2019

4 of 7 checks passed

code-review/reviewable 2 files left
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details

@gbrodman gbrodman deleted the gbrodman:dateTime branch Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.