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

Support of conversion to java.time classes #593

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chaosfirebolt
Copy link

- resolves issue cbeust#493
- unit tests
- supported - Instant, LocalDate, LocalDateTime, LocalTime, OffsetDateTime, OffsetTime, ZonedDateTime
- register converters in default factory
Copy link
Collaborator

@mkarg mkarg left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This is awesome!

Can you please check the issue I commented?

protected List<DateTimeFormatter> supportedFormats() {
return List.of(
DateTimeFormatter.ISO_LOCAL_DATE_TIME,
new DateTimeFormatterBuilder().appendPattern("dd-MM-yyyy").appendLiteral('T').append(DateTimeFormatter.ISO_LOCAL_TIME).toFormatter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Not really. ISO_DATE_TIME has the same date format as ISO_LOCAL_DATE_TIME - year-month-day, but it can handle offset and zone id, if they are present. The extra pattern I wrote has the date part in day-month-year format, which is also widely used as far as I know.

How would you like to handle that one? I would leave it as is, as it follows more strictly a local date time format (no offset/zone information), but I don't really insist on that.

Copy link
Author

Choose a reason for hiding this comment

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

@mkarg What are your thoughts on my previous comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will share them as soon as I have the time to.

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