search

FasterXML / jackson-datatype-joda

Extension module to properly support full datatype set of Joda datetime library
Apache License 2.0
140 stars 82 forks source link

Add support for Joda Money #114

Closed welandaz closed 4 years ago

welandaz commented 4 years ago

Hi, @cowtowncoder !

I'm not sure if this is a correct project to add a support for Joda Money datatypes (wasn't clear to me from Readme and project description), but here it is:)

Looking forward to a feedback!

cowtowncoder commented 4 years ago

@welandaz Hiya! First of all, thank you very much for your contribution. This should like a very useful improvement.

On where to add this: since many users of Joda date/time library are probably not using Joda Money (and possibly vice versa), I think it would probably make sense to actually create a separate datatype module. Or, possibly multi-Maven-project repo, and this as one of sub-projects; I have been thinking of consolidating some modules this way (perhaps something like jackson-datatypes-misc. Or, third option, possibly the best one: change this repo to be multi-module one and moving existing datatype module under joda-date, new one under joda-money. This would also make it easier to add support for other joda components (I think there are some others too?).

At first, it might be easiest to create a stand-alone repo, to be able to release this as part of Jackson 2.11, although option (3) could also work. If we start with standalone repo, I can also give you full owner access, which might be good initially to speed up process of fixes.

Regardless of option, one thing I would need before merging code (or creating new repo with it) is CLA (unless I have already asked for and received one). It is needed for all first-time contributions, but just once: all further contributions would be covered. CLA is here:

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

and the common way is to print it, fill & sign, scan / take photo, email to info at fasterxml dot com.

welandaz commented 4 years ago

Hi @cowtowncoder !

Sorry for the late response, the schedule has been a bit hectic these past days :(

Thanks a lot for having a look at the PR! I have sent the signed CLA to the email you've posted. If anything is missing or should be adjusted/edited/clarified - please, let me know.

I agree with your point, that not everyone who uses Joda date/time library is necessarily using Joda Money (and perhaps doesn't even want it to come as a transitive dependency). However, I thought the whole repo is supposed to represent the ultimate Jackson module for Joda libraries (if one registers JodaModule in Jackson configuration -> all of the serializers/deserializers are enabled).

Regarding the structural organization, I think standalone repo might be a bit too much, since the scope isn't that big. Option (3) sounds pretty good to me though. Is it the case then, that for Money objects there has to be a separate Jackson Module?

cowtowncoder commented 4 years ago

Right, it would be single Github repo, Maven project, but would produce 2 separate modules. It would be nice if we had some sort of concept that would allow convenience "bundles" of modules, but for now I think it'd either need to be single module providing support for all, or 2.

I think I'll ask on jackson-user list for feedback on this.

cowtowncoder commented 4 years ago

Due to timing constraints, will do this after 2.11.0 release, for 2.12. I think I will actually refactor this repo to do multi-maven project, with 2 modules. Going forward it will still be possible to combine to something like jackson-datatypes-misc or whatever, but this way can get joda-money support out without requiring bigger reorg.

cowtowncoder commented 4 years ago

@welandaz Ok so after going back and forth with this, I decided to create a new repo:

https://github.com/FasterXML/jackson-datatypes-misc

and add this on it, as well as move "org.json" datatype module. I will probably also move "jsr-353" datatype before 2.11, and Joda datetime itself maybe for 2.12 (the only reason not moving yet is that I want to reduce any risk due to refactoring there as it is one of more heavily used modules).

Since this required quite a bit of moving things around, I merged changes manually. I added you as the author in credits, and if you wish I can also give you write access to repository itself -- just let me know (I need to create a github team and give it access).

Plan is to get this module in for 2.11.0, and I'll need to add references from various documentation pages once this is done.

Thank you very much for this contribution: code is really clean and it is a pleasure to get that as a new standard Jackson extension module!

welandaz commented 4 years ago

Hi @cowtowncoder! Happy to help! :)

Sure, I'll be glad to have the write access to the new repo and be of assistance there.