Removed JodaTime dependency - migrated to java.time.

zapr-oss / druidry

Java based Druid Query Generator library

Apache License 2.0

193 stars 89 forks source link

Removed JodaTime dependency - migrated to java.time. #133

Open Jonarzz opened 4 years ago

Jonarzz commented 4 years ago

resolves #130

abhi-zapr commented 4 years ago

Hey, thanks for your contribution. As this is a breaking change and an alternative as same thing is achievable through joda-time and incubator-druid also uses joda-time as of now, let's have a further discussion if this patch should be merged or not.

Next steps :

Address PR comments
Reformat code if indentation is not equal everywhere. If you are using IntelliJ to code, you can use this style guide. https://s3-ap-southeast-1.amazonaws.com/zapper-static-data/intellij-java-google-style.xml
For newly added classes add Copyright in every file (ignore if already added) Thank you.

tunix commented 4 years ago

Hey, thanks for your contribution. As this is a breaking change and an alternative as same thing is achievable through joda-time and incubator-druid also uses joda-time as of now, let's have a further discussion if this patch should be merged or not.

I think we wouldn't be affected by this change even if Druid itself continues with joda-time, would we? (afterall, it's a matter of serialization & deserialization?)

On the other hand, one expects to see java.time support since the library (also druid itself) is built on top of Java 8 and not any prior releases. Having to add joda-time to a project that fully adapted java.time package is a bit frustrating. It'd really be nice to see this change in the upstream.

Jonarzz commented 4 years ago

On the other hand, one expects to see java.time support since the library (also druid itself) is built on top of Java 8 and not any prior releases. Having to add joda-time to a project that fully adapted java.time package is a bit frustrating

That's why in my opinion this change should be applied - Joda Time is just an unnecessary additional dependency.

Yet I do understand the hesitancy to merge this change as it would break all the code using Druidry and would require migration to java.time. Still - such change, if not required now (the code works fine as is and Joda API is all right), would make Druidry API better and the actual migration is not that hard (yet a docs update regarding that matter would be required).

abhi-zapr commented 4 years ago

That's why in my opinion this change should be applied - Joda Time is just an unnecessary additional dependency.

Yes totally agree. But as it's breaking change, we could plan to release in next major release 4.0 just like 3.0 was breaking due to few patches.

Jonarzz commented 4 years ago

That seems reasonable. I'll apply a patch regarding your review in a few days, thanks!

Jonarzz commented 2 years ago

@abhi-zapr, is the v4 still planned for release? If so, I could take care of the comments you provided.