Introduce cats-jvm and cats-js. #507

non · 2015-09-02T10:39:50Z

This commit does a bunch of things:

Adds a cats.macros.Platform object
Converts cats-macros to shared/jvm/js.
Converts other Cats modules to CrossType.Pure.
Moves JVM-specific code into cats-jvm.
Supports limited platform checking in laws/tests.

The new Platform object provides .isJvm and .isJs. These are
notable for being macros: they will be expanded at compile-time
to be either true or false. This allows scalac to eliminate
the "other" branch at compile-time, which will be useful to
us in laws/tests.

Moving forward, the goal is to only admit platform-specific code
(e.g. using java.io.*, or thread-blocking) into cats-jvm and
cats-js (which will not be required to provide the same APIs).
Other modules should be of CrossType.Pure (that is, they should
not have JVM- or JS-specific code in them).

The platform checking using .isJvm and .isJs should only be
used in cats-laws and cats-tests (although of course users of
the Cats library may use them however they like).

This commit does a bunch of things: * Adds a cats.macros.Platform object * Converts cats-macros to shared/jvm/js. * Converts other Cats modules to CrossType.Pure. * Moves JVM-specific code into cats-jvm. * Supports limited platform checking in laws/tests. The new Platform object provides .isJvm and .isJs. These are notable for being macros: they will be expanded at compile-time to be either true or false. This allows scalac to eliminate the "other" branch at compile-time, which will be useful to us in laws/tests. Moving forward, the goal is to only admit platform-specific code (e.g. using java.io.*, or thread-blocking) into cats-jvm and cats-js (which will not be required to provide the same APIs). Other modules should be of CrossType.Pure (that is, they should not have JVM- or JS-specific code in them). The platform checking using .isJvm and .isJs should only be used in cats-laws and cats-tests (although of course users of the Cats library may use them however they like).

non · 2015-09-02T10:42:47Z

As I hinted in the commit message, I think we should allow a limited amount of platform-specific code in laws/tests. The reason is that splitting those projects into shared/jvm/js feels really cumbersome, and IMO using things like Platform.isJvm to branch in tests is quite readable. However, the jvm module also has its own tests, so for code in cats-jvm we will put the tests there.

codecov-io · 2015-09-02T11:10:36Z

Current coverage is `64.77%`

Merging #507 into master will decrease coverage by -0.32% as of f04cfbc

@@            master    #507   diff @@
======================================
  Files          157     156     -1
  Stmts         2421    2419     -2
  Branches        66      67     +1
  Methods          0       0       
======================================
- Hit           1576    1567     -9
  Partial          0       0       
- Missed         845     852     +7

Review entire Coverage Diff as of f04cfbc

Powered by Codecov. Updated on successful CI builds.

adelbertc · 2015-09-03T05:27:48Z

build.sbt

@@ -251,7 +267,7 @@ lazy val commonScalacOptions = Seq(
  "-language:implicitConversions",
  "-language:experimental.macros",
  "-unchecked",
-  "-Xfatal-warnings",
+  //"-Xfatal-warnings",


Is something necessitating this?

Yes, this is necessary for now for cats-macros due to differences between 2.10 and 2.11. I think I can fix this though so I'll re-enable it when I do.

If you won't be fixing it in this PR, what about omitting this option from only the macros module?

adelbertc · 2015-09-03T05:36:57Z

Just a comment about turning off -X-fatal-warnings, I'd like to keep it on if possible. Other than that, awesome stuff - to more reliable builds!

non · 2015-09-03T13:37:42Z

Just to be clear -- I think the builds have already stabilized as far as JVM/JS split is concerned (the real culprits continue to be Travis flakiness and broken code). This is about trying to structure the project to encourage cross-platform support in a way that is easier to get right.

As a case study, consider the Task code that was added to core, despite using j.u.c classes. The structure here is supposed to make it easier to add the truly abstract, platform-independent code in core, and then have JVM-specific code in the separate cats-jvm project.

(This also means that if we can get this merged, I'd like to rework the feature/task branch to fix that abstraction leakage.)

/cc @stew

non · 2015-09-03T13:48:36Z

To expand on it a bit more -- in practice it seems like we haven't done a great job of dealing with projects whose code is split between shared/src, jvm/src, and js/src. So the strategy here was to limit us to a single project that does that (cats-macros) and then add in two more projects where all the JVM-specific and JS-specific code can go.

This might not be flexible enough (for example, if cats-state needed JVM-specific code we'd either have to make cats-jvm depend on cats-state, create a new module, or split up state). However, I think it is flexible enough, and a lot easier to keep straight.

non · 2015-09-04T00:49:50Z

Just re-enabled fatal warnings in everything except macros.

The "right" thing to do would be to split macros on 2.10/2.11 as well as JVM/JS, to remove the warnings. But that's a bit too fiddly to supress 2 warnings IMO. Once @milessabin finishes macro-compat (or someone else publishes something similar) we could consider using that.

ceedubs · 2015-09-04T11:41:45Z

I agree that it would be overkill to split macros on 2.10/2.11 to get rid of 2 warnings.

I do worry a bit about how complicated our project build/structure has gotten. It seems like it could be tough for potential contributors to jump in. This looks good to me though (with the disclaimer that I'm no expert on these matters). 👍

milessabin · 2015-09-09T21:22:22Z

macro-compat-1.0.0 is good to go ... have at it :-)

This commit adds a dependency (currently a snapshot) on scala-bricks, specifically bricks-platform. This means that all of Cats's cross-projects are now pure, which is quite nice. We shouldn't merge a snapshot dependency. Once there is a versioned scala-bricks release, we can add another commit to change to that version.

non · 2015-09-13T22:00:55Z

Hey folks, due to extensive work since this PR was created, there were conflicts that had to be updated.

I also pushed our scala-bricks dependency forward to the stable 0.0.1. This means that we no longer need to include the cats.macros.Platform object, since that exact object is included in scala-bricks. In the future I hope this kind of "general compatibility code" between JS and JVM will come from @inthenow's scala-bricks (which includes @milessabin's macro-compat).

I'd like to get this merged (or hear objections) relatively soon, since this PR is likely to conflict with any other changes to laws/tests that we want to make. To sum up the changes:

All Cats projects are "pure" Cross projects in SBT (meaning they only contain code that is valid on both the JVM and for JS).
There are special cats.jvm and cats.js projects where we can put useful instances and code for end users (and which none of the "pure" modules depend on).
All the tests pass reliably on the JVM and in JS.
We depend on scala-bricks, a compat library by @inthenow to make it easy to support both the JVM and JS (and which uses @milessabin's macro-compat library to make it easy to support 2.10 and 2.11 in macros).

What do you all think?

adelbertc · 2015-09-13T22:22:28Z

Looks like there is some merge conflicts, but to quote Cody, "This looks good to me though (with the disclaimer that I'm no expert on these matters)" 👍

I do like the idea of the scala-bricks library to simplify the build structure.

non · 2015-09-14T01:03:20Z

Thanks! This most recent commit should resolve the conflicts. If Travis goes green I'll merge it. 🐈

Introduce cats-jvm and cats-js.

non added the in progress label Sep 2, 2015

adelbertc reviewed Sep 3, 2015
View reviewed changes

Re-enable fatal warnings, except in the macros project.

620b33b

ghost mentioned this pull request Sep 6, 2015

Fix #458: Implement FutureInsantances for Scala-js #495

Closed

non added 5 commits September 12, 2015 23:11

Bump sbt-scalajs to 0.6.5.

ab7171f

Merge remote-tracking branch 'origin/master' into topic/js-jvm-projs

01b0624

Move Bimonad-related files to correct locations.

90a3a2d

Move to non-snapshot version of scala-bricks dependency.

2407a24

Merge remote-tracking branch 'origin/master' into topic/js-jvm-projs

f04cfbc

non added a commit that referenced this pull request Sep 14, 2015

Merge pull request #507 from non/topic/js-jvm-projs

5c2f182

Introduce cats-jvm and cats-js.

non merged commit 5c2f182 into master Sep 14, 2015

non removed the in progress label Sep 14, 2015

ceedubs mentioned this pull request Sep 14, 2015

Hook Eq up to scalactic/scalatest's equality #522

Merged

non deleted the topic/js-jvm-projs branch April 28, 2016 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce cats-jvm and cats-js. #507

Introduce cats-jvm and cats-js. #507

non commented Sep 2, 2015

non commented Sep 2, 2015

codecov-io commented Sep 2, 2015

adelbertc Sep 3, 2015

non Sep 3, 2015

ceedubs Sep 3, 2015

adelbertc commented Sep 3, 2015

non commented Sep 3, 2015

non commented Sep 3, 2015

non commented Sep 4, 2015

ceedubs commented Sep 4, 2015

milessabin commented Sep 9, 2015

non commented Sep 13, 2015

adelbertc commented Sep 13, 2015

non commented Sep 14, 2015

Introduce cats-jvm and cats-js. #507

Introduce cats-jvm and cats-js. #507

Conversation

non commented Sep 2, 2015

non commented Sep 2, 2015

codecov-io commented Sep 2, 2015

Current coverage is 64.77%

adelbertc Sep 3, 2015

Choose a reason for hiding this comment

non Sep 3, 2015

Choose a reason for hiding this comment

ceedubs Sep 3, 2015

Choose a reason for hiding this comment

adelbertc commented Sep 3, 2015

non commented Sep 3, 2015

non commented Sep 3, 2015

non commented Sep 4, 2015

ceedubs commented Sep 4, 2015

milessabin commented Sep 9, 2015

non commented Sep 13, 2015

adelbertc commented Sep 13, 2015

non commented Sep 14, 2015

Current coverage is `64.77%`