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

Remove catalysts dependency #2791

Merged
merged 6 commits into from
Apr 17, 2019

Conversation

travisbrown
Copy link
Contributor

The catalysts dependency has proven to be a drag on every update (right now it seems to be blocking a release for 2.13.0-RC1) and at this point it's almost completely unused, so I'm proposing that we just scrap it.

There's some dead code in LawTests that uses catalysts's TypeTagM, but it is unused and hasn't been touched since 2016, so I just scrapped it. Otherwise the only thing catalysts is used for is to check whether the current build is Scala.js in order to set appropriate test sizes.

In my view this really should be done with separate code trees, but to keep this PR simple I just introduced a new (package private) cats.platform.Platform that duplicates the ~dozen lines of code Cats needs from catalysts.

MiMa says this change is fine, all tests pass as usual with no changes (apart from the Platform import), etc.

r? @SethTisue @kailuowang

@kailuowang
Copy link
Contributor

kailuowang commented Apr 13, 2019

Thanks so much for taking on this. I believe this is the right thing to do.

@SethTisue
Copy link
Member

one less thing to worry about 👍

@travisbrown
Copy link
Contributor Author

I ran validateJVM and prePR and thought that would be enough to confirm that the build works, but apparently it's not. Fixing the alleycats issue now (and trying not to get extremely frustrated that alleycats is used as a top-level package and shares no path with cats…).

@codecov-io
Copy link

codecov-io commented Apr 13, 2019

Codecov Report

Merging #2791 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2791   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files         367      367           
  Lines        6926     6926           
  Branches      297      304    +7     
=======================================
  Hits         6530     6530           
  Misses        396      396
Impacted Files Coverage Δ
laws/src/main/scala/cats/laws/discipline/Eq.scala 33.33% <ø> (ø) ⬆️
...main/scala/cats/kernel/laws/PartialOrderLaws.scala 90.9% <ø> (ø)
...rc/main/scala/cats/kernel/laws/SemigroupLaws.scala 100% <ø> (ø)
.../scala/cats/kernel/laws/CommutativeGroupLaws.scala 100% <ø> (ø)
...ared/src/main/scala/cats/kernel/laws/package.scala 100% <ø> (ø)
...a/cats/kernel/laws/discipline/SemigroupTests.scala 100% <ø> (ø)
...kernel/laws/discipline/CommutativeGroupTests.scala 100% <ø> (ø)
...cala/cats/kernel/laws/discipline/MonoidTests.scala 100% <ø> (ø)
...cats/kernel/laws/discipline/SemilatticeTests.scala 100% <ø> (ø)
...cala/cats/kernel/laws/BoundedSemilatticeLaws.scala 100% <ø> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65bee09...97ac943. Read the comment docs.

Copy link
Member

@tpolecat tpolecat 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 great, thanks @travisbrown 👍

@kailuowang
Copy link
Contributor

I am on my phone, the code changes look good to me, but GitHub didn't really show the full path of the rename, so it appears that there is no actual path change. I am curious what they are.

@travisbrown travisbrown mentioned this pull request Apr 14, 2019
@kailuowang
Copy link
Contributor

replaced by #2792

@kailuowang kailuowang closed this Apr 17, 2019
@kailuowang kailuowang reopened this Apr 17, 2019
@kailuowang
Copy link
Contributor

@travisbrown I spoke too soon in #2792, there is a conflict and can't be resolved throught github ui. Would it be too much to ask you to resolve it?

@travisbrown
Copy link
Contributor Author

@kailuowang Merged, thanks.

@kailuowang kailuowang merged commit 007b261 into typelevel:master Apr 17, 2019
@kailuowang kailuowang added this to the 2.0.0-M1 milestone Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants