Add two more options for returning a DateTime - Instant and Date #26
Conversation
Provide automatic serialization for UTC timestamps returned as either java.time.Instant or java.util.Date.
|
Any chance this can be pulled in? I would also like to use |
| import java.time.DateTimeException; | ||
| import java.time.OffsetDateTime; | ||
| import java.time.ZonedDateTime; | ||
| import java.time.*; |
There was a problem hiding this comment.
There are zero wildcard imports in this repository. I recommend to match the repository's code style.
| @@ -32,6 +31,12 @@ public String serialize(Object input) throws CoercingSerializeException { | |||
| offsetDateTime = (OffsetDateTime) input; | |||
| } else if (input instanceof ZonedDateTime) { | |||
| offsetDateTime = ((ZonedDateTime) input).toOffsetDateTime(); | |||
| } else if (input instanceof Instant) { | |||
| offsetDateTime = ((Instant) input).atOffset(ZoneOffset.UTC); | |||
| } else if (input instanceof LocalDateTime) { | |||
There was a problem hiding this comment.
I don't think it makes sense to ever convert a LocalDateTime to a single moment in time without extra context. The reason for using the LocalDateTime type in Java is to semantically indicate that the time does not represent a universal moment in time but rather a conceptual calendar date and time.
It cannot represent an instant on the time-line without additional information such as an offset or time-zone.
That's why these methods exist:
LocalDateTime.atOffset(zoneOffset)LocalDateTime.atZone(zoneId)
Passing a LocalDateTime to this serialize method probably indicates there are bugs around due to misunderstanding dates. Otherwise, if the caller truly means for a LocalDateTime to represent a UTC date/time, then the caller can call .atOffset(ZoneOffset.UTC) first.
| } else if (input instanceof LocalDateTime) { | ||
| offsetDateTime = ((LocalDateTime) input).atOffset(ZoneOffset.UTC); | ||
| } else if (input instanceof Date) { | ||
| offsetDateTime = ((Date) input).toInstant().atOffset(ZoneOffset.UTC); | ||
| } else if (input instanceof String) { | ||
| offsetDateTime = parseOffsetDateTime(input.toString(), CoercingSerializeException::new); | ||
| } else { |
There was a problem hiding this comment.
What about the parseValue method below? That one still only has cases for OffsetDateTime, ZonedDateTime, and String.
|
@fotoetienne - Curious on the timeline for this one? Would really love to use this with our environment using Instant. |
|
@bbakerman This PR looks stale. I am happy to submit a PR to fix the comments so support for Instant and Date can be added. Let me know if you are open to this. |
Yeah I think the LocalDateTime code should be removed but supporting Instant and Date (as UTC) makes sense |
|
This PR #39 has an explcit LocalTime scalar which I think its more correct in terms of indicating that its a ISO local time without timezone. Often this will allow you to SHOOT yourself but some people do want just |
|
Thanks for opening a PR, and sorry it has taken so long to reply. Similarly to #52, I discussed with the team and we decided to not include Instant, Date and LocalDateTime. For Instant and LocalDateTime, the challenge is neither has an offset as @andrewsf said. It would be better for the user to choose the offset rather than the library to assume an offset. For Date, we'd rather not support this outdated Java class. |
Provide automatic serialization for UTC timestamps returned as
either java.time.Instant or java.util.Date.