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

Change IntervalDeserializer to respect input timezone when enabled #105

Closed
wants to merge 1 commit into from
Closed

Change IntervalDeserializer to respect input timezone when enabled #105

wants to merge 1 commit into from

Conversation

Woodz
Copy link
Contributor

@Woodz Woodz commented Sep 20, 2018

Bump dependency on joda-time to v2.9 to access Interval.parseWithOffset
Change IntervalDeserializer to use Interval.parseWithOffset and
override timezone only if shouldAdjustToContextTimeZone is set or
explicit timezone is specified in the format
Add unit tests for IntervalDeserializer to cover the 3 scenarios:

Bump dependency on joda-time to v2.9 to access Interval.parseWithOffset
Change `IntervalDeserializer` to use `Interval.parseWithOffset` and
 override timezone only if `shouldAdjustToContextTimeZone` is set or
 explicit timezone is specified in the format
Add unit tests for `IntervalDeserializer` to cover the 3 scenarios:
- Timezone explicitly provided in deserialization format
- Context timezone enabled
- Neither context timezone enabled nor explicit timezone provided
Resolve: #104
@Woodz
Copy link
Contributor Author

Woodz commented Apr 16, 2019

@cowtowncoder any chance you can review and merge this PR?

@cowtowncoder
Copy link
Member

@Woodz sorry, this slipped through the cracks.

@cowtowncoder
Copy link
Member

Hmmh. While I otherwise like the change, requirement to use Joda 2.9 needs some thought. I realize it has been around for 4 years now, but upgrade is still a breaking change for some users.

Jackson 2.10 does actually use 2.9.9 as build dependency, but should work with older versions.

So this needs to go in a new minor version: I think 2.11 is fine (2.10.0 just went out, for better or worse).

With that, I'd be almost ready to merge. Just one more thing: unless I have asked for it already, CLA would be needed from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the easiest way is to print, fill, sign & scan, email to info at fasterxml dot com.
With that, I can proceed with the new branch, merging.

Thank you again for contributing this, apologies for slow follow up.

@cowtowncoder cowtowncoder added 2.11 and removed 2.10 labels Oct 5, 2019
@cowtowncoder
Copy link
Member

Sent a note to jackson-dev: unless anything drastic heard, planning on merging this in 2.11 branch. Would be great to rebase, but even if not I can just manually patch as the change is nicely compact.

@Woodz
Copy link
Contributor Author

Woodz commented Oct 5, 2019

@cowtowncoder I have emailed over the CLA as requested and have rebased this commit onto master to go into the 2.11 release. Unfortunately I no longer have access to the previous branch (I have since left the organisation under which I created it), so forked a new repo under my personal account and created a PR #109 to replace this one. Apologies for the inconvenience of changing PRs

@Woodz Woodz closed this Oct 5, 2019
@cowtowncoder
Copy link
Member

@Woodz thank you for the new PR -- not a problem at all, and makes my life easier. Also thank you for sending CLA so quickly.

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