• Removes joda from encoding/decoding

• common

• mysql

• postgres

continuation of #229

Getting following in CI build.. :/

docker: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.

Hi, thanks for the PR!
There was some interest for it in the past. For reference see #131 and #157.
Ideally, if we could have both implementation as a non-breaking change it would be preferred.
But I understand this might complicate things a lot.
Since this is a breaking change we will need to bump the major version when merged.

I could try to have Joda Time supported as well. But the thing is, do we really want to continue supporting it?
I prefer the major version upgrade. But seems odd going from 1.1.7 to 2.0.0.
Suggestions on how to move forward with this please?

Bump version to 2.0.0 sounds reasonable to me.
Please rebase changes from master again, build should work now in CI.

Note that some tests are still failing on CI.

I fixed most. Just having issues understanding logic behind

assertThat(dateTime.millis).isGreaterThan(915779106000L)

can you help, @oshai ?

I didn't wrote it originally, but from reading the code I think it checks the following:

• set timestamp that includes mills/micros
• check that the timestamp millis conversion is greater than the second before and smaller from the second after

There might be a problem either with the timezone or the millis if I have to guess.

I noticed something, postgres is returning timestamps in UTC string, even when I insert it with non UTC offset.
The tests are expecting non UTC offset. So I changed a bit and is actually testing if the two timestamps are equal as OffsetDateTime
Is this correct approach?
I am guessing postgres returning UTC string is related to server settings (which I am not really changing). Is this correct assumption?

I noticed something, postgres is returning timestamps in UTC string, even when I insert it with non UTC offset.
The tests are expecting non UTC offset. So I changed a bit and is actually testing if the two timestamps are equal as OffsetDateTime
Is this correct approach?
I am guessing postgres returning UTC string is related to server settings (which I am not really changing). Is this correct assumption?

According to https://www.postgresql.org/docs/9.1/datatype-datetime.html 8.5.2. Date/Time Output:

The default is the ISO format
and it looks like:

ISO | ISO 8601/SQL standard | 1997-12-17 07:37:16-08

(without time zone)

Conversion on test makes sense (to see it's the same date as inserted). It's just not clear how this was changed from pervious impl.

I still need to review all changes, but it will take some time.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR.
I left some comments in code and have some more general things:

• I saw that in some cases there is no test coverage: do you think it's an issue with the framework, or can you please add tests to cover those cases?
• I saw in many places move from millis to nano. Is it because java 8 uses nano and joda/db use millis?

About the migration to 2.x: I am thinking if we can add a suggestion to users how to do that more easily? Do you have any suggestion?

I saw that in some cases there is no test coverage: do you think it's an issue with the framework, or can you please add tests to cover those cases?

Can you point me to which cases are you talking about specifically?

I saw in many places move from millis to nano. Is it because java 8 uses nano and joda/db use millis?

Yes. Java uses nano, joda uses millis, db micro(at least in mysql)

Can you point me to which cases are you talking about specifically?

Look at LocalDateTimeEncoderDecoder:17 and see more examples in 'files changed' with the warning: Added line #Lxx was not covered by tests

@dragneelfps before merging there was still one answered question in the review about DATETIME(6).

In addition, will you have some time to improve test coverage?

Stale pull request message

I will merge this PR soon. Will create 1.x branch before that.

Thanks for the PR!
I merged it and will release a new version and update docs soon.