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 java.time arbitraries on Scala.js & Native #830

Merged
merged 7 commits into from
Mar 15, 2022

Conversation

armanbilge
Copy link
Member

We can also use these instances on Scala.js via scala-java-time. This would be helpful for http4s/http4s#4938.

I was surprised that most of the tests for this project (not just specific to java.time) are only run on the JVM, is there a particular reason for this? It would be great to run more of these on more platforms.

Thanks!

@ashawley
Copy link
Contributor

When time support was added in #734, it was known that Scala.js could be supported. The issue was deferred was all.

@armanbilge
Copy link
Member Author

Absolutely! Well if you are open to it now it would be really helpful :)

@armanbilge
Copy link
Member Author

@ashawley
Copy link
Contributor

Yeah, it does that on occasion. Re-starting...

@armanbilge
Copy link
Member Author

I think it happened again 😅

@armanbilge
Copy link
Member Author

armanbilge commented Jul 30, 2021

Sadly, I have to close this :( @mpilquist pointed out to me that adding a dependency to scala-java-time here creates a circular dependency between it and scalacheck. The right thing to do is to provide these arbitraries in scala-java-time itself.

@armanbilge armanbilge closed this Jul 30, 2021
@armanbilge armanbilge reopened this Nov 13, 2021
@armanbilge
Copy link
Member Author

Revisiting this, I realized that actually we don't need scala-java-time here. So there is no circular dependency!

Comment on lines 28 to 32
private final lazy val minJavaDuration: Duration =
Duration.ofSeconds(Long.MinValue)

private final lazy val maxJavaDuration: Duration =
Duration.ofSeconds(Long.MaxValue, 999999999L)

implicit final lazy val arbJavaDuration: Arbitrary[Duration] =
implicit final lazy val arbJavaDuration: Arbitrary[Duration] = {
val minJavaDuration = Duration.ofSeconds(Long.MinValue)
val maxJavaDuration = Duration.ofSeconds(Long.MaxValue, 999999999L)
Arbitrary(Gen.choose(minJavaDuration, maxJavaDuration))
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The Scala.js linker/optimizer was unable to elide the Duration reference for some reason. This fixed it.

@ashawley
Copy link
Contributor

ashawley commented Dec 7, 2021

Revisiting this, I realized that actually we don't need scala-java-time here. So there is no circular dependency!

What changed? I don't follow Scala.js developments closely.

@armanbilge
Copy link
Member Author

Hmm, nothing changed? Just my own understanding. In my initial attempt I was adding an unneeded dependency.

@ashawley
Copy link
Contributor

ashawley commented Dec 7, 2021

Ok, I was wondering if scala-java-time or similar was recently merged with Scala.js internals. If so, was curious if it required a minimum version of Scala.js for these time bindings.

@armanbilge
Copy link
Member Author

So FTR my confusion was about compile time vs linking/runtime. Since we are not testing these arbitraries, we don't need java.time implementations for Scala.js, only the types signatures for compilation. These are provided by the JDK itself.

@ashawley
Copy link
Contributor

ashawley commented Dec 7, 2021

Ok, thanks for the clarification.

@ashawley
Copy link
Contributor

ashawley commented Dec 7, 2021

Do you know if Scala native is similarly able to support Java time API in this way? Should we do that here, as well, while we're at it?

@armanbilge
Copy link
Member Author

Yes, I think so. My understanding has been that SJS and native have a similar model wrt compile vs link/run-time. Let me do that here, actually it should simplify this PR!

In any case, it seems the java.time implementations for native are provided by a different library, which would not create a circular dependency with scalacheck.
https://github.com/ekrich/sjavatime

@ashawley
Copy link
Contributor

ashawley commented Dec 7, 2021

Great, thanks for looking into it.

@armanbilge armanbilge changed the title Support java.time arbitraries on Scala.js Support java.time arbitraries on Scala.js & Native Dec 7, 2021
@armanbilge
Copy link
Member Author

Cool, that seems to have worked :) good idea!

@armanbilge
Copy link
Member Author

@rossabaker while you're here this one is good for http4s 😉

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I barely understand scala.JS and native not at all, but it's well argued and got a green CI run.

@SethTisue SethTisue merged commit 669a7d7 into typelevel:main Mar 15, 2022
armanbilge added a commit to armanbilge/scalacheck that referenced this pull request Apr 8, 2022
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.

4 participants