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

Remove joda-time library and use Java 8 time instead #355

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chriskilding
Copy link

Remove joda-time library and use Java 8 time instead.

@chriskilding
Copy link
Author

This change might be an alternative solution to #142

@chriskilding
Copy link
Author

Starting with a largely mechanical line-by-line translation of Joda Time API calls to their Java 8 Time equivalents, to see what works and what doesn't.

@chriskilding
Copy link
Author

chriskilding commented Nov 17, 2020

First note is that Java 8's DateTimeFormatter#appendFraction accepts a maximum fraction width of 9 rather than 12. This is just a limitation of Java 8 time that we'd have to live with.

Several of the tests fail only because they use fraction widths longer than 9, so we'd need to delete those.

@chriskilding
Copy link
Author

chriskilding commented Nov 17, 2020

Just 3 test failures to go...

+HHmm offsets are rejected while +HH:mm offsets are allowed (in other words, the new parsing has strict RFC3339 offset format requirements everywhere, whereas parsing with Joda time was lenient):

Gradle suite > Gradle test > com.github.fge.jsonschema.format.common.DateTimeTest > instanceIsCorrectlyAnalyzed[0]("2012-12-02T13:05:00+0100", true, null, null) FAILED
    org.mockito.exceptions.verification.NoInteractionsWanted

Java time tolerates the 30th February whereas Joda rejects it:

Gradle suite > Gradle test > com.github.fge.jsonschema.format.draftv3.DateTest > instanceIsCorrectlyAnalyzed[5]("1922-02-30", false, string "1922-02-30" is invalid against requested date format(s) yyyy-MM-dd, {"value":"1922-02-30","expected":"yyyy-MM-dd"}) FAILED
    org.mockito.exceptions.verification.WantedButNotInvoked

Java time tolerates "24:00:00" whereas Joda rejects it:

Gradle suite > Gradle test > com.github.fge.jsonschema.format.draftv3.TimeTest > instanceIsCorrectlyAnalyzed[3]("24:00:00", false, string "24:00:00" is invalid against requested date format(s) HH:mm:ss, {"value":"24:00:00","expected":"HH:mm:ss"}) FAILED
    org.mockito.exceptions.verification.WantedButNotInvoked

.appendLiteral(':')
.appendValue(ChronoField.SECOND_OF_MINUTE, 2)
.appendOptional(secFracsParser)
.appendZoneOrOffsetId();
Copy link
Author

Choose a reason for hiding this comment

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

Could be the cause of (overly?) strict offset validation, causing +0100 to be rejected.

@adetunjiakintundeakinde

Do you need help with the PR. I am willing to help as we need this change for one of our projects

@chriskilding
Copy link
Author

I do need suggestions on what should be done about the remaining 3 tests - whether each Java 8 time behaviour difference is acceptable (so we need to update or delete the relevant tests) or not (so we need to fine tune the implementation). The library maintainers need to provide their opinion on this as well.

Also if you can think of relevant missing test cases then please do suggest/add them :)

@chriskilding
Copy link
Author

@Capstan would you (or one of the other maintainers) be able to take a look at this?

@chriskilding
Copy link
Author

chriskilding commented Jan 21, 2021

@ajorg-aws would you (or another maintainer) be able to review this?

@ajorg-aws
Copy link
Contributor

@ajorg-aws would you (or another maintainer) be able to review this?

Hello! I'm not a maintainer, just a contributor. The commit you see from me was contributed in #351

@chriskilding
Copy link
Author

@Capstan would you (or another maintainer) be able to take a look at this?

@xp-vit
Copy link

xp-vit commented Mar 12, 2021

@chriskilding That is a really nice and necessary change! Shall we contact maintainers?

@chriskilding
Copy link
Author

@huggsboson I believe you are one of the other maintainers in the java-json-tools org. Would you be able to review this and give me some design guidance to resolve the last failing tests, and then we can see about merging this?

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.

4 participants