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

Add Kernel instances for java-time datatypes #3910

Closed
wants to merge 1 commit into from
Closed

Add Kernel instances for java-time datatypes #3910

wants to merge 1 commit into from

Conversation

diesalbla
Copy link
Contributor

Issue 3766 proposed integrating cats-time into cats, and adding instances for the data types in java.time, which is after all just as standard as the Scala library or UUID.

All the instances, as defined in cats-time, just use the natural equality, hash code, or comparable implementations defined in the Java library. To ease the definition, within the companion objects of the typeclasses Eq, Order, and Hash, we define new "mixin" traits that provide implementations of each type-class based on the Any-hash, Any-eq, or Comparable.

With that, each class in java.time has an instance of the kernel typeclasses written in two lines.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Choose a reason for hiding this comment

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

This is just in kernel? Should this have forwarders so cats imports get this automatically or am I missing them?

trait JavaTimeInstances {

implicit final val catsKernelStdOrderForduration
: Hash[Duration] with Order[Duration] with CommutativeGroup[Duration] =
Copy link
Contributor

Choose a reason for hiding this comment

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

we aren't testing the commutative group are we? I don't see where those laws are checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the problem I am having with some of the tests is that, if it generates very large values, it is causing overflows. Not sure how we should address this problem?

Copy link
Contributor Author

@diesalbla diesalbla Aug 9, 2021

Choose a reason for hiding this comment

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

I was able to fix it by defining a generator for the Duration class itself. It is just copied from the one for FiniteDuration. @johnynek Would that be enough?

@djspiewak
Copy link
Member

Once this is compiling on ScalaJS, we need to double-check that downstream projects are not forced to take on the Java Time shim dependency.

@@ -25,6 +25,7 @@ trait AllInstances
with FutureInstances
with HashInstances
with InvariantMonoidalInstances
with JavaTimeInstances
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just in kernel? Should this have forwarders so cats imports get this automatically or am I missing them?

@ChristopherDavenport IIUC this should add the instances to the core itself too

In [Issue 3766](#3766) it was
proposed to add java.time instances to cats itself, considering that
`java.time` is just as standard as UUID or the Scala library.

Within the companion objects of the typeclasses `Eq`, `Order`, and
`Hash`, we define new "mixin" traits that provide implementations
of each type-class based on the Any-hash, Any-eq, or Comparable.
These are only used to define simple instances of many typeclasses.
With that, each class in `java.time` has an instance of the kernel
typeclasses written in two lines, and they all fit in one file.

We also add instances of Show in the `core` module, which are
just based on the `toString` method.

- Duration Test: add a generator for durations
@diesalbla
Copy link
Contributor Author

Once this is compiling on ScalaJS, we need to double-check that downstream projects are not forced to take on the Java Time shim dependency.

The model followed here is similar to that of java.util.UUID, which is also included in the kernel, in the main branch.

@ChristopherDavenport
Copy link
Member

Once this is compiling on ScalaJS, we need to double-check that downstream projects are not forced to take on the Java Time shim dependency.

This is super super important. Time is an enormous increase to JS file size where the prospect of having it in a JS file is generally referred to as crazy by the community.

@armanbilge
Copy link
Member

armanbilge commented Nov 26, 2021

I was pointed here by @joroKr21 in #3871 (comment).

Actually, if @diesalbla or someone else wants to, I think we can move forward with this. Here's how:

  1. Move all the java.time instances into shared code (as is done in this PR).
  2. Don't add a dependency to scala-java-time. It's only needed for linking (not compiling) and only if java.time classes are actually used.
  3. Don't test java.time instances in the JS test suite. If the test suite still runs, this demonstrates that the linker was able to remove the (unused) references to java.time.
  4. (Optional) Create another test (sbt) project/module with scala-java-time as a test dependency, and use it to test the java.time instances on JS.

What this achieves:

  • if downstreams don't care about java.time, this PR will have no impact on them. They won't have scala-java-time on the classpath, and as long as they don't use java.time instances in their code the linker will not complain, as demonstrated in (3).
  • If a downstream does want to use java.time stuff, it's all there! They'll just have to explicitly add a dependency to scala-java-time (or any alternative shims of this stuff!) to get their code to link.

Risks:

  • Because code using java.time will always compile (regardless of the availability of implementations for it to link against), having these instances will make it easier for libraries to unwittingly use this code without realizing the impact on their downstreams. I mean, this is true right now, because anyone can use java.time anytime anywhere, not test their code, and only find out a few downstreams later when they link what consequences that has. The difference is that currently, the lack of these instances for Scala.js in cats might create enough of a compile-time stumbling block that they get properly informed on the situation :)

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.

5 participants