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

Minimizing typeclass surface in cats-kernel #1997

Merged
merged 11 commits into from
Oct 31, 2017

Conversation

denisrosset
Copy link
Contributor

As per #1995

@codecov-io
Copy link

codecov-io commented Oct 30, 2017

Codecov Report

Merging #1997 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1997      +/-   ##
==========================================
+ Coverage   95.24%   95.26%   +0.01%     
==========================================
  Files         301      301              
  Lines        4922     4919       -3     
  Branches      123      125       +2     
==========================================
- Hits         4688     4686       -2     
+ Misses        234      233       -1
Impacted Files Coverage Δ
kernel/src/main/scala/cats/kernel/Hash.scala 90% <ø> (ø) ⬆️
core/src/main/scala/cats/instances/eq.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/WriterT.scala 94.62% <100%> (ø) ⬆️
core/src/main/scala/cats/data/Nested.scala 96.49% <100%> (ø) ⬆️
...e/src/main/scala/cats/instances/partialOrder.scala 100% <100%> (ø) ⬆️
laws/src/main/scala/cats/laws/discipline/Eq.scala 94.23% <100%> (ø) ⬆️
...rnel/src/main/scala/cats/kernel/PartialOrder.scala 90.9% <100%> (+2.67%) ⬆️
core/src/main/scala/cats/instances/order.scala 100% <100%> (ø) ⬆️
kernel/src/main/scala/cats/kernel/Eq.scala 93.75% <100%> (-0.19%) ⬇️
kernel/src/main/scala/cats/kernel/Order.scala 83.33% <100%> (-0.46%) ⬇️
... and 1 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 1f0cba0...b313bc2. Read the comment docs.

@denisrosset denisrosset changed the title Moved Eq/PartialOrder/Order.on implementation to companion object. Minimizing typeclass surface in cats-kernel Oct 30, 2017
@LukaJCB
Copy link
Member

LukaJCB commented Oct 30, 2017

Can you rebase with master? :)

@kailuowang
Copy link
Contributor

this wil fail Mima test , need to add some exceptions here

@kailuowang kailuowang modified the milestones: 1.0.0, 1.0.0-RC1 Oct 30, 2017
Copy link
Contributor

@oscar-stripe oscar-stripe left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes!

johnynek
johnynek previously approved these changes Oct 30, 2017
@kailuowang
Copy link
Contributor

[info] Compiling 78 Scala sources to /home/travis/build/typelevel/cats/tests/.js/target/scala-2.11/test-classes...
[error] /home/travis/build/typelevel/cats/tests/src/test/scala/cats/tests/SortedSetSuite.scala:17: value reverse is not a member of cats.kernel.PartialOrder[scala.collection.immutable.SortedSet[Int]]
[error] checkAll("PartialOrder[SortedSet[Int]].reverse", PartialOrderTests(PartialOrder[SortedSet[Int]].reverse).partialOrder)
[error] ^
[error] /home/travis/build/typelevel/cats/tests/src/test/scala/cats/tests/SortedSetSuite.scala:18: value reverse is not a member of cats.kernel.PartialOrder[scala.collection.immutable.SortedSet[Int]]
[error] checkAll("PartialOrder[SortedSet[Int]].reverse.reverse", PartialOrderTests(PartialOrder[SortedSet[Int]].reverse.reverse).partialOrder)
[error] ^
[error] two errors found
[error] (testsJS/test:compileIncremental) Compilation failed

@denisrosset
Copy link
Contributor Author

@kailuowang Rebased with the latest master, should solve the SortedSetSuite problem.

PS: my git-fu is minimal, how should I combine the above commits to avoid polluting the history?

@denisrosset
Copy link
Contributor Author

We should also remove the tests corresponding to the old .reverse.reverse test, as we no longer perform the optimization that the reverse of the reverse is the original object.

Note: I could restore this optimization by pattern matching on the type of derived Order, at the price of adding new classes.

@kailuowang
Copy link
Contributor

Its okay to have multiple trivial commits in a PR. We will squash merge into one commit. I myself don't see it necessary to optimize .reverse.reverse. Testing it against the law doesn't hurt though right?

@denisrosset
Copy link
Contributor Author

It doesn't hurt, but it's unnecessary, as the .reverse.reverse test was covering an optimization that is no longer there, so we are basically testing that -(-x) = x as the Order.reverse method cannot be overloaded. Your call.

@kailuowang kailuowang merged commit af7354c into typelevel:master Oct 31, 2017
rintcius added a commit to rintcius/radixtree that referenced this pull request Aug 24, 2018
In this commit, the "on" method was removed from cat's Eq trait.
The "by" method which was using that method was reimplemented..
rintcius added a commit to rintcius/radixtree that referenced this pull request Aug 24, 2018
In that PR, the "on" method was removed from cat's Eq trait.
The "by" method which was using that method was reimplemented..
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.

6 participants