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 TimeZone consideration for DateMidnight and Interval #67

Closed
wants to merge 1 commit into from

Conversation

islanderman
Copy link

This pull request is to address #58

Essentially there are only following datatypes that need to handle TimeZone from DeserializationContext:

  1. DateTime: Already handled. Consolidate the way it is handled.
  2. DateMidnight: Adding TimeZone information into DateMidnight
  3. Interval: Adding TimeZone information into Interval

The rest are not relevant since they are either defaulted as UTC or conceptually not applicable. I still add unit test cases just to make sure.

Also adding more unit test cases for modified code. All unit test cases are passing. Backward compatibility should not be an issue.

@islanderman
Copy link
Author

Close in favor of 726ed70

@cowtowncoder
Copy link
Member

Oh shoot, I did not notice this PR before working on the other check in. My apologies.

Do you think additional tests are still valid? Likewise, I only added TimeZone forcing for Interval, and not the other types.

@islanderman
Copy link
Author

No problem at all.  The test cases should still be valid.  Feel free to pick them or I can make another pull request later.

@cowtowncoder
Copy link
Member

Sounds good, I hope to match them when I get a chance. Right now finalizing 2.6.0, improvements could go in a patch.

@islanderman
Copy link
Author

@cowtowncoder I tried to merge my test cases with your change, but this following test failed:

        MAPPER.setTimeZone(TimeZone.getTimeZone("Europe/Paris"));

        Interval interval = MAPPER.readValue(quote("1396439982-1396440001"), Interval.class);
        assertEquals(1396439982, interval.getStartMillis());
        assertEquals(1396440001, interval.getEndMillis());
        assertEquals(ISOChronology.getInstance(DateTimeZone.forID("Europe/Paris")), interval.getChronology());

Essentially this TimeZone is based on DeserializationContext, which is set via ObjectMapper. It looks like your change honors #66 instead. Either way it is fine, but I believe #66 should also consider setTimeZone as well.

@cowtowncoder
Copy link
Member

@islanderman You are probably correct here, and my patch is incomplete. Feel free to fix if you have time; or file a separate issue if not? (so it will be fixed eventually)

@islanderman
Copy link
Author

Sure. I'll file a separate issue and fix it with another pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants