-
Notifications
You must be signed in to change notification settings - Fork 7
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
Java 8 Date Time support: typeclasses #6
Conversation
the quirks that go with it
Current coverage is 100%
|
LGTM! |
|
||
package object instances { | ||
object joda extends JodaInstances | ||
object j8 extends J8Instances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDK8 may be a better known acronym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
LGTM! |
|
||
trait J8Instances { | ||
|
||
implicit val j8ForDuration: ScalaCheckDateTimeInfra[ZonedDateTime, Duration] = new ScalaCheckDateTimeInfra[ZonedDateTime, Duration] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to replace it for the following ?
implicit object j8ForDuration extends ScalaCheckDateTimeInfre[ZonedDateTime, Duration] {
I have done this before, but I do not know if they are better than a val
. It could save an anonymous class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had issues in the past with type inference when doing things that way (admittedly with more complex types than this). I seem to recall that implicit val...
is the suggested way to do it, though I can't find any evidence to back that up. Doing it my way allows explicit setting of the desired type, which I like, so I'll probably stick with that for now.
LGTM |
Thanks for the reviews - I'm going to refactor |
This PR is a first stab at including support for other datetime libraries as well as Joda. This allows the generators to work with Java 8's
java.time
library.You can now call
genDateTimeWithinRange
with, for instance, a JodaDateTime
andPeriod
, or a Java8ZonedDateTime
andDuration
, and it should return the appropriate type.How this works:
genDateTimeWithinRange
takes an implicitScalaCheckDateTimeInfra
type, which is parameterized on the date class and the "range". As long as an implicit exists for that date/range pair, then this works. For this PR, JodaDateTime
andPeriod
and Java8ZonedDateTime
andDuration
are included, but there's no reason not to include more, such as JodaDateTime
with JodaDuration
, or the other incarnations of Java8'sDateTime
s that are notZoned
.To the end-user, the generators such as
genDateTimeWithinRange
should "just work", without needing to understandScalaCheckDateTimeInfra
- just having the correct imports should be fine.By example, to work with Java 8, these imports are needed:
and
... instances.joda._
exists too.Right now, the
ScalaCheckDateTimeInfra
expects to be able to convert whatever classes are provided to millis. I'm pretty sure I can take another look at this and remove that restriction (especially as Java 8 has nanosecond granularity). Another reason is that this PR uncovered some crazy stuff with Java 8's library, such as getting the milliseconds of a Date that can't be represented by milliseconds throws an arithmetic exception"Again, I'd love some feedback if possible, especially around the naming (I hate
ScalaCheckDateTimeInfra
) and the general approach taken.Resolves #2