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 extension jackson-jr-extension-javatime to support (some) Java 8 date/time types #111

Merged
merged 13 commits into from
Feb 19, 2024

Conversation

Shounaks
Copy link
Contributor

Sorry about previous tries, I believe this might suffice.
Regarding Issue #100

@cowtowncoder
Copy link
Member

Quick note: I fixed issue with CI failures (a regression in jackson-core), so now all tests pass as expected.

@cowtowncoder
Copy link
Member

First of all: thank you for contributing this, @Shounaks !
This sounds good overall; I'll need to go through it but I think the idea works well.
About the only immediate thing I noticed is that we need Moditect definition under

 ..../src/moditect/module-info.java

(see examples of f.ex jr-annotation-support/)

Aside from that, one thing we need is CLA, from:

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

and it only needs to be sent once before the first contribution. Usually it's easiest to print, fill & sign, scan/photo, email to cla at fasterxml dot com.

Thank you again; I am looking forward to getting this merged for inclusion in 2.17.0 release!

pom.xml Outdated Show resolved Hide resolved
@cowtowncoder
Copy link
Member

Ok, one more thing: I think main README.md should be modified to add brief explanation of the new extension. And there should be small jr-extension-datetime/README.md as well.

I will also remove JDK 20 references; package is still JDK 8 compatible for Jackson 2.x

README.md Outdated Show resolved Hide resolved
@cowtowncoder
Copy link
Member

Looks good; if we can get CLA sorted out I think I should be able to merge this!

@Shounaks Shounaks marked this pull request as ready for review February 15, 2024 01:32
@Shounaks
Copy link
Contributor Author

There is one more thing I would like to add, if you permit. that would be custom formatter support for this functionality
image
so using this, the developer would have a control on the format data is is coming on. I am sharing its patch file as well, feel free to tell me what you think about it.

Adding_Custom_Formatter_Support_for_LocalDateTime.patch

@cowtowncoder
Copy link
Member

Sure, the idea of configurability sounds reasonable. One thing I would suggest, however, is to not call formatter DEFAULT_FORMATTER and assume it is used for everything, since if and when adding new types they will need different formatters.

So I would probably add separate setter method for configuring DateTimeFormatter for LocalDateTime.

.... actually, I only now realize that the provider probably should be renamed too, not to be just for LocalDateTime. I think it should be something more generic.

@cowtowncoder cowtowncoder changed the title Using Module for DateTime Extension. Add extension jackson-jr-extension-javatime to support (some) Java 8 date/time types Feb 19, 2024
@cowtowncoder cowtowncoder merged commit 9e64875 into FasterXML:2.17 Feb 19, 2024
4 checks passed
@cowtowncoder
Copy link
Member

First of all: thank you @Shounaks for contributing this! It will be in Jackson 2.17.0 rc1 to be released soon.

One thing: I Merged before remembering to ask or one thing -- basic set of unit tests would be nice, if possible (via different PR)?

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