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

issue 58 - NUMBER_INT should be specified when deserializing LocalDate as EpochDays #139

Closed
wants to merge 2 commits into from

Conversation

kupci
Copy link
Member

@kupci kupci commented Sep 24, 2019

fix for issue #58 NUMBER_INT should be specified when deserializing LocalDate as EpochDays (unless lenient)

@kupci
Copy link
Member Author

kupci commented Sep 24, 2019

Needs a 'strict' test.

@cowtowncoder
Copy link
Member

Looks good -- I'll add some comments on adding @since tags on new methods.

@cowtowncoder
Copy link
Member

Quick question: this is against master: I assume this is because change might not make it in time for 2.10? If so that's fine; I'll create 2.11 branch right after 2.10.0 release, and probably want to rebase against that. I can of course try cherry-picking to backport, even if merged against master. But I assume we'd like this for 2.x series, if possible?

@kupci
Copy link
Member Author

kupci commented Sep 24, 2019

Cool, thanks for review. I'll make requested changes tonight. I have been going to upstream/master more out of habit, but I can certainly create PR against 2.10 if tonight CST is ok. One thing I need to figure out, to improve test, is how to send two config overrides, quick Google search and JavaDoc didn't reveal the technique, so I was thinking of creating a small test class and adding annotation there, which I've seen in other tests.

@kupci kupci marked this pull request as ready for review September 24, 2019 18:52
@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 24, 2019

Ok, I think I did config overrides in a test yesterday. Method is different between 2.10 and 3.0, due to rewrite, but 2.10 does have builder-based approach now too I think.
I think change was in .... LocalDateDeserializerTest? Should be possible to chain.

Having said that, annotation is fine too of course.

@kupci
Copy link
Member Author

kupci commented Sep 25, 2019

I'll close this and create a PR for 2.10 instead. The annotation worked, for some reason the override was overriding the override, but I see it works for the property settings (just the .with(), so it's possible I need to call the 'with all config overrides' method or something like that.

@kupci kupci closed this Sep 25, 2019
@cowtowncoder
Copy link
Member

As per my email comments, rebase for 2.10 branch is fine; just need to make sure that all existing constructors/methods in base class still exist in patch (deprecated).
And when merging forward (master / 3.0), remove such deprecated methods, only needed within 2.x.

@kupci
Copy link
Member Author

kupci commented Oct 5, 2019

@cowtowncoder Ok if I keep this one as-is, since it's ready, and then I will backport to 2.10?

@kupci kupci reopened this Oct 5, 2019
@cowtowncoder
Copy link
Member

Quick note: I just created 2.11 branch, as that will be needed here. It could have gone in 2.10.0, since minor versions can have some changes in behavior, but I try not to do such changes in patch versions. Problem being that there is small (but existing) chance of breakage.
I will try to keep 2.11 dev cycle much shorter than 2.10, 2.11 pre-release candidates maybe going out by December.

So, merge to master, cherry-pick to 2.11; or base off 2.11, either work.

@kupci kupci closed this Oct 8, 2019
@kupci kupci deleted the add-shape-58 branch October 14, 2019 01:06
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