Add some instances we were missing. #1878

non · 2017-08-31T06:37:15Z

This commit adds support for some types:

Ordering, PartialOrdering, and Equiv
scala.concurrent.duration.Duration
scala.collection.immutable.Queue

The ordering types should be relatively uncontroversial.

I chose to support Duration even though arguably FiniteDuration is a
bit better behaved. This isn't the worst group we support, and I feel
like it's possible that folks will really benefit from these instances.

Queue is probably the most interesting immutable data structure we
didn't already support. I based its instances on those for List,
rewriting things to use .isEmpty and .dequeue instead of
pattern-matching.

We could potentially try to support some of the other concrete
collection types (e.g. HashMap, TreeMap, IntMap, SortedMap, etc.)
which might not be a bad idea. I haven't done that here.

I added tests for Queue and Duration, but have not (yet) tested the
pretty trivial contravariant instances.

Fixes #1853.

This commit adds support for some types: - Ordering, PartialOrdering, and Equiv - scala.concurrent.duration.Duration - scala.collection.immutable.Queue The ordering types should be relatively uncontroversial. I chose to support Duration even though arguably FiniteDuration is a bit better behaved. This isn't the worst group we support, and I feel like it's possible that folks will really benefit from these instances. Queue is probably the most interesting immutable data structure we didn't already support. I based its instances on those for List, rewriting things to use .isEmpty and .dequeue instead of pattern-matching. We could potentially try to support some of the other concrete collection types (e.g. HashMap, TreeMap, IntMap, SortedMap, etc.) which might not be a bad idea. I haven't done that here. I added tests for Queue and Duration, but have not (yet) tested the pretty trivial contravariant instances.

non · 2017-08-31T07:01:29Z

Looks like this is failing a binary compatibility check.

kailuowang · 2017-08-31T14:18:49Z

There are a few other tickets blocked by kernel bin compat, (#1712, #1527). @denisrosset suggested in gitter that it's no big deal for the spire community to break in the 1.0 release, I just asked in the algebird gitter room. @johnynek suggested that he'd rather not break. Is that something we can reevaluate? If not what's the alternative? (move all these to cats.core or a new module?)

non · 2017-08-31T14:29:43Z

@kailuowang I think if we leave DurationInstances and QueueInstances out of the kernel's all.scala that should fix the kernel issues, no? I'll try that here.

codecov-io · 2017-08-31T14:35:40Z

Codecov Report

Merging #1878 into master will increase coverage by 0.19%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master    #1878      +/-   ##
==========================================
+ Coverage   94.97%   95.17%   +0.19%     
==========================================
  Files         241      248       +7     
  Lines        4199     4349     +150     
  Branches      106      122      +16     
==========================================
+ Hits         3988     4139     +151     
+ Misses        211      210       -1

Impacted Files	Coverage Δ
...a/cats/laws/discipline/NonEmptyTraverseTests.scala	`100% <ø> (ø)`	⬆️
...in/scala/cats/laws/discipline/ReducibleTests.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/instances/duration.scala	`0% <0%> (ø)`
core/src/main/scala/cats/instances/ordering.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/queue.scala	`100% <100%> (ø)`
...ain/scala/cats/laws/discipline/TraverseTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/order.scala	`100% <100%> (ø)`	⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.06% <100%> (-0.13%)`	⬇️
... and 20 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 d85cc86...adcb06b. Read the comment docs.

non · 2017-08-31T14:37:15Z

So with that commit we maintain binary compatibility for kernel.

kailuowang · 2017-08-31T14:40:44Z

so shall we have a cats.instances.extra to place all the new instances helpers or something?

non · 2017-08-31T14:59:19Z

@kailuowang Let's wait and see how many there are. For now I think we should just add them to all.scala commented out, and then make a decision just before the 1.0 release.

(Looks like last error was formatting, pushing a fix now.)

non · 2017-08-31T15:00:45Z

In cases where we are adding instances for a new type, the approach I used should be good I think. For adding instances to an existing type (i.e. where we already have a trait in kernel and can't add to it without breaking compat), let's just put them in core with a comment that they could be in kernel except for bincompat.

What do you think @johnynek?

kailuowang · 2017-08-31T15:02:02Z

@non 👍 on deciding later. I just created a dedicated issue #1879 for it.

kailuowang · 2017-08-31T15:30:58Z

core/src/main/scala/cats/instances/ordering.scala

+      /** Derive an `Ordering` for `B` given an `Ordering[A]` and a function `B => A`.
+       *
+       * Note: resulting instances are law-abiding only when the functions used are injective (represent a one-to-one mapping)
+       */


On a related note (not suggesting we should change anything here), we do have an Inject with laws, user could use it to test if a function f: A => B is injective, by creating a prj: B => Option[A] and an Inject[A, B] using these two, then testing the law against this Inject instance.

{ implicit val inject = new Inject[A, B] { val inj = f val prj : B => Option[A] = ??? } checkAll("Inject[A, B]", InjectTests[A, B].inject) }

If you think this would be a good idea, we could potentially add it to the cats-testkit.

kailuowang · 2017-08-31T15:40:43Z

core/src/main/scala/cats/instances/partialOrdering.scala

+import cats.functor.Contravariant
+
+trait PartialOrderingInstances {
+  implicit val catsFunctorContravariantForPartialOrdering: Contravariant[PartialOrdering] =


Note sure if this instance is tested.

kailuowang · 2017-08-31T15:49:28Z

core/src/main/scala/cats/instances/queue.scala

+
+trait QueueInstances extends cats.kernel.instances.QueueInstances {
+
+  implicit val catsStdInstancesForQueue: Traverse[Queue] with Alternative[Queue] with Monad[Queue] with CoflatMap[Queue] =


A bunch of these overrides are not tested. I am not sure how big a deal it is since they don't seem to be tested for the list instance either.

non · 2017-08-31T15:52:45Z

@kailuowang I can probably get us test coverage on all of this if we're willing to wait until later today or tonight.

kailuowang · 2017-08-31T15:55:43Z

kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala

@@ -93,6 +136,7 @@ class LawTests extends FunSuite with Discipline {
  laws[OrderLaws, List[String]].check(_.order)
  laws[OrderLaws, Vector[Int]].check(_.order)
  laws[OrderLaws, Stream[Int]].check(_.order)
+  laws[OrderLaws, Queue[Int]].check(_.order)


I couldn't find OrderLaws test with Duration?

johnynek

this is fine from a binary break-age point of view. We don't directly depend on the instances in algebird.

johnynek · 2017-08-31T18:12:24Z

core/src/main/scala/cats/instances/ordering.scala

+trait OrderingInstances {
+
+  implicit val catsFunctorContravariantForOrdering: Contravariant[Ordering] =
+    new Contravariant[Ordering] {


can't this be ContravariantCartesian as well? Just use left to right ordering?

Yup. Initially I was uncertain about adding this but ended up going for it.

johnynek · 2017-08-31T18:12:59Z

core/src/main/scala/cats/instances/partialOrdering.scala

+
+trait PartialOrderingInstances {
+  implicit val catsFunctorContravariantForPartialOrdering: Contravariant[PartialOrdering] =
+    new Contravariant[PartialOrdering] {


can't this be contravariantCartesian using left to right ordering?

Yup. Initially I was uncertain about adding this but ended up going for it.

non · 2017-09-01T06:55:11Z

Added a bunch more tests, and ended up doing some refactors to other parts of the tests to remove some boilerplate.

It's sort of terrifying to realize that we're closing in on 1.0 and there's still so much potential polish we could do. 😛

kailuowang · 2017-09-01T13:22:02Z

laws/src/main/scala/cats/laws/discipline/Arbitrary.scala

-      PartialOrder.from[A]((_: A, _: A) => -1.0),
-      PartialOrder.from[A]((_: A, _: A) => 0.0),
-      PartialOrder.from[A]((_: A, _: A) => 1.0)))
+    Arbitrary(getArbitrary[Int => Double].map(f => new PartialOrder[A] {


👍 this makes a lot more sense

kailuowang · 2017-09-01T14:54:40Z

@non I wonder if you and/or @ceedubs have time to quickly skim through all the code in cats-core to spot all the obvious polish needed before 1.0 release?

non · 2017-09-01T17:12:26Z

The previous test failure was legit: by testing some of the Foldable and Traversable methods against their references, I found two instances (Map[K, ?] and NonEmptyVector) which were treating negative indices incorrectly.

non · 2017-09-01T19:54:04Z

I think this is finally ready-to-go test coverage-wise.

johnynek · 2017-09-01T20:13:31Z

👍

Thanks for tackling this before 1.0!

non added the in progress label Aug 31, 2017

non mentioned this pull request Aug 31, 2017

Contravariant functor instance for scala.math.Ordering #1853

Closed

Maintain kernel bincompat.

ab44661

kailuowang mentioned this pull request Aug 31, 2017

Decide on a solution for adding new features to cats-kernel avoiding breaking bin-compat. #1879

Open

Fix formatting.

e98b09f

kailuowang reviewed Aug 31, 2017

View reviewed changes

johnynek reviewed Aug 31, 2017

View reviewed changes

Add a bunch of tests.

75031ba

Trailing comma, lol.

06ca15b

kailuowang reviewed Sep 1, 2017

View reviewed changes

Get tests running under 2.10, and fix some bugs.

746357e

erik-stripe added 2 commits September 1, 2017 13:31

Appease styler :/

0d5ed9e

Reenable lighterweight test.

3453cd1

Enable more reference tests.

adcb06b

kailuowang approved these changes Sep 1, 2017

View reviewed changes

non merged commit 7e9a183 into typelevel:master Sep 2, 2017

non removed the in progress label Sep 2, 2017

kailuowang mentioned this pull request Sep 12, 2017

ApplicativeError: catchNonFatal, catchNonFatalEval and fromTry are not covered by laws #1908

Open

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

kailuowang added the enhancement label Oct 15, 2017

barambani mentioned this pull request Jun 15, 2020

Issue 3353: Investigate antisymmetry failure for UpperBounded for Duration #3470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some instances we were missing. #1878

Add some instances we were missing. #1878

non commented Aug 31, 2017

non commented Aug 31, 2017

kailuowang commented Aug 31, 2017

non commented Aug 31, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

non commented Aug 31, 2017

kailuowang commented Aug 31, 2017

non commented Aug 31, 2017

non commented Aug 31, 2017 •

edited

Loading

kailuowang commented Aug 31, 2017 •

edited

Loading

kailuowang Aug 31, 2017 •

edited

Loading

kailuowang Aug 31, 2017

kailuowang Aug 31, 2017

non commented Aug 31, 2017

kailuowang Aug 31, 2017

johnynek left a comment

johnynek Aug 31, 2017

non Sep 1, 2017

johnynek Aug 31, 2017

non Sep 1, 2017

non commented Sep 1, 2017

kailuowang Sep 1, 2017 •

edited

Loading

kailuowang commented Sep 1, 2017

non commented Sep 1, 2017

non commented Sep 1, 2017

johnynek commented Sep 1, 2017


		trait QueueInstances extends cats.kernel.instances.QueueInstances {

		implicit val catsStdInstancesForQueue: Traverse[Queue] with Alternative[Queue] with Monad[Queue] with CoflatMap[Queue] =

Add some instances we were missing. #1878

Add some instances we were missing. #1878

Conversation

non commented Aug 31, 2017

non commented Aug 31, 2017

kailuowang commented Aug 31, 2017

non commented Aug 31, 2017

codecov-io commented Aug 31, 2017 • edited Loading

Codecov Report

non commented Aug 31, 2017

kailuowang commented Aug 31, 2017

non commented Aug 31, 2017

non commented Aug 31, 2017 • edited Loading

kailuowang commented Aug 31, 2017 • edited Loading

kailuowang Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang Aug 31, 2017

Choose a reason for hiding this comment

kailuowang Aug 31, 2017

Choose a reason for hiding this comment

non commented Aug 31, 2017

kailuowang Aug 31, 2017

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

johnynek Aug 31, 2017

Choose a reason for hiding this comment

non Sep 1, 2017

Choose a reason for hiding this comment

johnynek Aug 31, 2017

Choose a reason for hiding this comment

non Sep 1, 2017

Choose a reason for hiding this comment

non commented Sep 1, 2017

kailuowang Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Sep 1, 2017

non commented Sep 1, 2017

non commented Sep 1, 2017

johnynek commented Sep 1, 2017

codecov-io commented Aug 31, 2017 •

edited

Loading

non commented Aug 31, 2017 •

edited

Loading

kailuowang commented Aug 31, 2017 •

edited

Loading

kailuowang Aug 31, 2017 •

edited

Loading

kailuowang Sep 1, 2017 •

edited

Loading