-
Notifications
You must be signed in to change notification settings - Fork 56
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 Scala Native support for core #105
Conversation
Remove specs2-scalacheck test dependency from core Rewrite core Specification in plain Scalacheck
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.
Thanks!
I'd say priority for the ecosystem is releasing discipline-1.0.0 so we can release cats-2.0.0. I'd prefer we move on #106 first, which will cause a couple of light merge conflicts here. I commented on where I anticipate them. But I also don't see any reason that this would be incompatible with being introduced in a subsequent 1.x release.
@@ -1,9 +1,27 @@ | |||
language: scala | |||
dist: trusty |
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.
trusty is EOL. We'd want xenial, which I just switched to in #106.
@@ -102,18 +111,27 @@ lazy val root = project.in(file(".")) | |||
specs2DisciplineJVM | |||
) | |||
|
|||
lazy val discipline = crossProject(JSPlatform, JVMPlatform) | |||
lazy val discipline = crossProject(JSPlatform, JVMPlatform, NativePlatform) |
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 just renamed this to core
in #106. Didn't think it would make work for anyone. Oops.
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'll rebase it on top and use the new names.
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.
Is it ok to wait for #106 to be merged? I can put my changes on top of it then.
for additive groups ${additiveGroup} | ||
for multiplicative groups ${multiplicativeGroup} | ||
""" | ||
class DisciplineSpec extends Properties("A RuleSet should compute the properties correctly") { |
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.
👍 This looks like a positive change, with or without scala-native support. It uses a dependency already on hand.
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.
Could somebody please provide this part as an extra PR? Happy to merge that now.
First of all, @lolgab, I would like to thank you for this contribution. I appreciate you working on scala-native support. Now the "but" part. I'm a little concerned about using a forked dependency of ScalaCheck. There's currently a discussion on its future maintenance, and I'd like to at least wait for this. If it still turns out to be impossible to get the native support merged upstream, we can talk again. PS: I'm happy to be overruled by other Typelevel members on this. |
I don't like the use of a forked artifact too. I tried to ask on scalacheck gitter, on the github issue but nobody of the maintainers published the Scala Native artifact. (Scala Native support is merged on master branch of Scalacheck, it's only giving |
As far as I know (from what @non said) only Rickard currently can run releases. |
1.14.1 with Scala Native support is out. I'll pick up this PR again. |
Superceded by #106
Remove specs2-scalacheck test dependency from core
Rewrite core Specification in plain Scalacheck