Add implicit conversions from java.time classes to plotly LocalDateTime #178
Conversation
| implicit def fromJavaOffsetDateTime(javaOffsetDateTime: OffsetDateTime): PlotlyLocalDateTime = | ||
| fromJavaLocalDateTime(javaOffsetDateTime.toLocalDateTime) | ||
|
|
||
| implicit def fromJavaZonedDateTime(javaZonedDateTime: ZonedDateTime): PlotlyLocalDateTime = | ||
| fromJavaLocalDateTime(javaZonedDateTime.toLocalDateTime) | ||
|
|
There was a problem hiding this comment.
Worth noting that there are some potential gotchas here around converting zoned datetimes caused by dropping the zone component. For example, converting a list of datetimes with differing zones/offsets may lead to unexpected results. I'm happy to add a Scaladoc warning if needed.
There was a problem hiding this comment.
Feel free to mention it in a scaladoc comment if you wish, yes.
There was a problem hiding this comment.
Thanks @tmccarthy!
Maybe the non-ambiguous conversions (from LocalDateTime, Instant, and LocalDate, I guess) could be added directly in the companion of plotly.element.LocalDateTime, so that users don't need the plotly.JavaTime._ import.
| implicit def fromJavaOffsetDateTime(javaOffsetDateTime: OffsetDateTime): PlotlyLocalDateTime = | ||
| fromJavaLocalDateTime(javaOffsetDateTime.toLocalDateTime) | ||
|
|
||
| implicit def fromJavaZonedDateTime(javaZonedDateTime: ZonedDateTime): PlotlyLocalDateTime = | ||
| fromJavaLocalDateTime(javaZonedDateTime.toLocalDateTime) | ||
|
|
There was a problem hiding this comment.
Feel free to mention it in a scaladoc comment if you wish, yes.
Sounds good. I avoided this because it takes a bit of work to get around the lack of |
|
Oh, I had missed that |
|
Nah I can do it! |
… companion, and split out potentially-unsafe conversions so that they require an explicit import.
| package plotly.element | ||
|
|
||
| trait PlotlyJavaTimeConversions { | ||
| // Empty since the java.time classes are not available in ScalaJS |
There was a problem hiding this comment.
Worth noting that another solution here would be to add a dependency on scala-java-time from the core-js project. Let me know if you'd like to do this.
There was a problem hiding this comment.
I guess it's fine this way, let's not add an extra dependency…
| * It will generally be safer to convert your data to `java.time.LocalDateTime` in the appropriate timezone/offset, | ||
| * and then use the `PlotlyJavaTimeConversions.fromJavaLocalDateTime` implicit conversion. | ||
| */ | ||
| object UnsafeImplicitConversions { |
There was a problem hiding this comment.
These implicit conversions now need to be explicitly imported using something like:
import plotly.element.LocalDateTime.UnsafeImplicitConversions._I like this approach, although I am open to feedback on the UnsafeImplicitConversions name.
|
That looks great, merging! |
This PR adds implicit conversions from the
java.timeclasses toplotly.element.LocalDateTime. These conversions are similar to those provided for Joda time inplotly.Joda. They are included only in the JVM artefact.