Add Order instance for NonEmptySet (#2779) #2787

jatcwang · 2019-04-11T22:33:12Z

Add Serializable tests for NonEmptySet instances

Thank you for contributing to Cats!

This is a kind reminder to run sbt prePR and commit the changed files, if any, before submitting.

codecov-io · 2019-04-12T21:26:11Z

Codecov Report

Merging #2787 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##           master   #2787      +/-   ##
=========================================
- Coverage   94.21%   94.2%   -0.02%     
=========================================
  Files         368     368              
  Lines        6948    6949       +1     
  Branches      303     303              
=========================================
  Hits         6546    6546              
- Misses        402     403       +1

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptySet.scala	`97.59% <66.66%> (-1.2%)`	⬇️
laws/src/main/scala/cats/laws/discipline/Eq.scala	`33.33% <0%> (ø)`	⬆️
...in/scala/cats/kernel/instances/short/package.scala
...ain/scala/cats/kernel/instances/unit/package.scala
...ain/scala/cats/kernel/instances/uuid/package.scala
...n/scala/cats/kernel/instances/bitSet/package.scala
...main/scala/cats/kernel/instances/set/package.scala
...in/scala/cats/kernel/instances/queue/package.scala
...main/scala/cats/kernel/instances/map/package.scala
...scala/cats/kernel/instances/duration/package.scala
... and 44 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 b504edc...de79fe2. Read the comment docs.

kailuowang · 2019-04-17T15:13:59Z

core/src/main/scala/cats/data/NonEmptySet.scala

@@ -417,3 +420,29 @@ sealed abstract private[data] class NonEmptySetInstances {
    def combine(x: NonEmptySet[A], y: NonEmptySet[A]): NonEmptySet[A] = x | y
  }
 }
+
+sealed abstract private[data] class NonEmptySetInstances0 extends NonEmptySetInstances1 {
+  implicit def catsDataOrderForNonEmptySetReal[A](implicit A: Order[A]): Order[NonEmptySet[A]] =


nit, is that Real suffix still needed?

Not sure. catsDataOrderForNonEmptySet is the name of the existing one (which I removed the implicit keyword from). I can play around with it and see if it breaks bin compat

did you mean private[data] def catsDataEqForNonEmptySet[A: Order]: ?

yep since NonEmptySetInstances extends NonEmptySetInstances0

:) catsDataOrderForNonEmptySet is different from catsDataEqForNonEmptySet.

🤦‍♂️ well if MiMa is happy then I'm happy :)
I'm gonna try remove the old catsDataEqForNonEmptySet and see what happens

sorry I wasn't clear. I meant that you didn't need to add a Real suffix, because it didn't conflict with anything else. It's the right approach to remove the implict keyword for catsDataEqForNonEmptySet , but you didn't need to add private[data] since the class is already private[data]

yes I got you. What I was also going to do is to remove the original catsDataEqForNonEmptySet, since there's already a method named the same in NonEmptySetInstances1. (I think JVM's use of invokevirtual call means it won't break bin compat - so it'll be both source and binary compatible)

kailuowang · 2019-04-17T15:16:51Z

core/src/main/scala/cats/data/NonEmptySet.scala

@@ -19,6 +19,8 @@ package data

 import cats.instances.sortedSet._
 import cats.kernel._
+import cats.syntax.order._
+import cats.instances.sortedSet._


duplicated from 3 lines above

kailuowang · 2019-04-17T21:35:33Z

I would like to propose :
We don't need two new classes, just one NonEmptySetInstances0 is enough, you can place the eq instance there and the order instance in NonEmptySetInstances
I think this change is BC but Mima will raise a false alarm. We need to add a mima exception
here https://github.com/typelevel/cats/blob/master/build.sbt#L288

ProblemFilters.exclude[IncompatibleMethTypeProblem]("cats.data.NonEmptySetInstances.catsDataEqForNonEmptySet")

To make sure we indeed didn't break BC, we need to add a line calling the instance def here https://github.com/typelevel/cats/blob/master/binCompatTest/src/main/scala/catsBC/MimaExceptions.scala similar to the other tests there.

jatcwang · 2019-04-29T22:36:40Z

Done (Sorry I was AFK for a week). Simplified the instances hierachy as suggested and added a different test class for testing binary package private instances.

- Add Serializable tests for NonEmptySet instances

…mpatibility. Simplify NonEmptySet instances hierachy

jatcwang · 2019-05-24T20:16:41Z

CI is passing again @kailuowang 😄

kailuowang · 2019-05-28T13:44:29Z

tests/src/test/scala/cats/tests/NonEmptySetSuite.scala

+             SerializableTests.serializable(PartialOrder[NonEmptySet[ListWrapper[Int]]]))
+
+    Eq[NonEmptySet[ListWrapper[Int]]]
+    PartialOrder[NonEmptySet[ListWrapper[Int]]]


These two lines are unnecessary, no?

These instance tests are written based on https://github.com/typelevel/cats/blob/4f92efedd2c30534c07b5f75a72b22e3e1ba9b13/tests/src/test/scala/cats/tests/NonEmptyListSuite.scala and from what I can gather is that it's to test that an Eq and PartialOrder instance can be resolved when an Order instance is in scope.

Do you think they are redundant?

The resolution is already tested in the law and serialization tests

kailuowang · 2019-05-28T13:47:45Z

tests/src/test/scala/cats/tests/NonEmptySetSuite.scala

+    // NOTE: Order[NonEmptySet[A]] instance depends on Order[SortedSet[A]]. SortedSet only has Order instance which depends on Order[A] instance
+    //       and thus the only way we can get PartialOrder[NonEmptySet[A]] is via construction of Order[NonEmptySet[A]]
+    checkAll("NonEmptySet[ListWrapper[Int]]", PartialOrderTests[NonEmptySet[ListWrapper[Int]]].partialOrder)
+    checkAll("PartialOrder[NonEmptySet[ListWrapper[Int]]]",


Maybe I am missing something here, but this is covered by the Order test above, since we don't have a different way to construct a PartialOrder, not sure if we need this test?

you're right. Removed thx

kailuowang · 2019-05-28T13:49:08Z

binCompatTest/src/test/scala/cats/data/CatsDataMimaException.scala

+package cats.data
+import cats.implicits._
+
+object CatsDataPackagePrivateMimaExceptions {


My bad. I should've checked that mima no longer falsely claims that this type of change breaks BC. So this is no longer needed? Would you mine removing it?

kailuowang self-requested a review April 17, 2019 01:52

kailuowang reviewed Apr 17, 2019

View reviewed changes

jatcwang and others added 4 commits May 23, 2019 22:18

Add Order instance for NonEmptySet (#2779)

80b770e

- Add Serializable tests for NonEmptySet instances

remove old catsDataEqForNonEmptySet. Name Order instance properly

1872a53

Add CatsDataMimaException to test cats.data package private binary co…

72db5fd

…mpatibility. Simplify NonEmptySet instances hierachy

Fix compile error

9cc1efa

kailuowang reviewed May 28, 2019

View reviewed changes

remove redundant tests

de79fe2

LukaJCB approved these changes Jun 5, 2019

View reviewed changes

kailuowang approved these changes Jun 11, 2019

View reviewed changes

kailuowang merged commit 71bbf39 into typelevel:master Jun 11, 2019

kailuowang added the enhancement label Jun 11, 2019

LukaJCB mentioned this pull request Jul 3, 2019

Order instance for NonEmptySet #2779

Closed

kailuowang added this to the 2.0.0-RC1 milestone Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Order instance for NonEmptySet (#2779) #2787

Add Order instance for NonEmptySet (#2779) #2787

jatcwang commented Apr 11, 2019

codecov-io commented Apr 12, 2019 •

edited

Loading

kailuowang Apr 17, 2019

jatcwang Apr 17, 2019

kailuowang Apr 17, 2019

jatcwang Apr 17, 2019

kailuowang Apr 17, 2019

jatcwang Apr 17, 2019

kailuowang Apr 17, 2019

jatcwang Apr 17, 2019

jatcwang Apr 29, 2019

kailuowang Apr 17, 2019 •

edited

Loading

jatcwang Apr 29, 2019

kailuowang commented Apr 17, 2019 •

edited

Loading

jatcwang commented Apr 29, 2019

jatcwang commented May 24, 2019

kailuowang May 28, 2019

jatcwang May 29, 2019

kailuowang May 30, 2019

jatcwang Jun 5, 2019

kailuowang May 28, 2019

jatcwang May 29, 2019

kailuowang May 28, 2019

jatcwang May 29, 2019

Add Order instance for NonEmptySet (#2779) #2787

Add Order instance for NonEmptySet (#2779) #2787

Conversation

jatcwang commented Apr 11, 2019

codecov-io commented Apr 12, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Apr 17, 2019 • edited Loading

jatcwang commented Apr 29, 2019

jatcwang commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 12, 2019 •

edited

Loading

kailuowang Apr 17, 2019 •

edited

Loading

kailuowang commented Apr 17, 2019 •

edited

Loading