More instances for prod #1402

pepegar · 2016-10-07T18:57:56Z

Creating more instances for Prod:

codecov-io · 2016-10-07T19:43:23Z

Current coverage is 91.68% (diff: 100%)

Merging #1402 into master will decrease coverage by <.01%

@@             master      #1402   diff @@
==========================================
  Files           240        240          
  Lines          3610       3632    +22   
  Methods        3546       3568    +22   
  Messages          0          0          
  Branches         63         63          
==========================================
+ Hits           3310       3330    +20   
- Misses          300        302     +2   
  Partials          0          0

Powered by Codecov. Last update a392654...867b2a2

pepegar · 2016-10-13T09:35:34Z

Hi @adelbertc is there something else I can do here?

adelbertc · 2016-10-13T17:50:42Z

tests/src/test/scala/cats/tests/ProdTests.scala

+    checkAll("MonadCombine[Prod[ListWrapper, ListWrapper, ?]]", SerializableTests.serializable(MonadCombine[Prod[ListWrapper, ListWrapper, ?]]))
+  }
+
+  test("order") {


Can you check OrderLaws as well? https://github.com/typelevel/cats/blob/master/kernel-laws/src/main/scala/cats/kernel/laws/OrderLaws.scala

adelbertc · 2016-10-13T17:51:26Z

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

+    def F: Order[F[A]] = FF
+    def G: Order[G[A]] = GF
+  }
+  implicit def catsDataShowForProd[F[_], G[_], A](implicit FF: Show[F[A]], GF: Show[G[A]]): Show[Prod[F, G, A]] = new ProdShow[F, G, A] {


This one can probably be moved up to ProdInstances right

adelbertc · 2016-10-13T17:51:50Z

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

+}
+
+private[data] sealed abstract class ProdInstances9 {
+  implicit def catsDataOrderForProd[F[_], G[_], A](implicit FF: Order[F[A]], GF: Order[G[A]]): Order[Prod[F, G, A]] = new ProdOrder[F, G, A] {


This can probably be moved up to one sub class down from the Eq instance

kailuowang · 2016-10-18T15:13:56Z

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

 }

 private[data] sealed abstract class ProdInstances0 extends ProdInstances1 {
  implicit def catsDataMonoidKForProd[F[_], G[_]](implicit FF: MonoidK[F], GG: MonoidK[G]): MonoidK[λ[α => Prod[F, G, α]]] = new ProdMonoidK[F, G] {
    def F: MonoidK[F] = FF
    def G: MonoidK[G] = GG
  }
+
+  implicit def catsDataOrderForProd[F[_], G[_], A](implicit FF: Order[F[A]], GF: Order[G[A]]): Order[Prod[F, G, A]] = new ProdOrder[F, G, A] {


As @adelbertc pointed out, Order instance should have higher priority than Eq since it's more specific. I think we should move this instance to ProdInstances and the Eq instance down here.

Yes, sure! I misunderstood @adelbertc in the previous comment. Now it's done.

Thanks very much!

kailuowang · 2016-10-19T14:14:40Z

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

+    def G: MonadCombine[G] = GF
+  }
+}
+


In this particular case, to ensure the more specific instance having the highest priority, we basically want to ensure that instances of typeclasses who are lower in the inheritance tree are also lower in the instance trait inheritance tree. For example, Monad inherits Functor and thus is more "specific" than Functor, so it should be in an instance trait with higher priority (0 - 3) than where Functor is (4).
So, we need to rearrange the instances to that order.
MonadCombine <: Altenative
MonadCombine <: Monad <: Applicative <: Apply <: Functor
Traverse <: Foldable
Traverse <: Functor

Thanks for pointing that out, I didn't think on that!

I've done the changes in 000a583

kailuowang · 2016-10-20T16:27:04Z

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

-    def F: MonadCombine[F] = FF
-    def G: MonadCombine[G] = GF
+private[data] sealed abstract class ProdInstances7 {
+  implicit def catsDataAlternativeForProd[F[_], G[_]](implicit FF: Alternative[F], GG: Alternative[G]): Alternative[λ[α => Prod[F, G, α]]] = new ProdAlternative[F, G] {


Thanks! We are getting close.
how about we do
ProdInstances: MonadCombine, Order, Show ,Traverse, Contravariant
ProdInstances0: Alternative, Monad, Eq, Foldable
ProdInstances1: MonoidK, Applicative
ProdInstances2: SemigroupK, Apply
ProdInstances3: Functor

I might make some mistakes here too.
You may find this diagram helpful
https://camo.githubusercontent.com/28086d65ea5688b722ef4c19ee70e1974c5f4cb0/687474703a2f2f766972656f2e6f72672f746d702f74797065636c61737365732e737667

kailuowang

Thanks! We are getting close.
how about we do
ProdInstances: MonadCombine, Order, Show ,Traverse, Contravariant
ProdInstances0: Alternative, Monad, Eq, Foldable
ProdInstances1: MonoidK, Applicative
ProdInstances2: SemigroupK, Apply
ProdInstances3: Functor

I might make some mistakes here too.
You may find this diagram helpful https://camo.githubusercontent.com/28086d65ea5688b722ef4c19ee70e1974c5f4cb0/687474703a2f2f766972656f2e6f72672f746d702f74797065636c61737365732e737667

pepegar · 2016-10-20T16:50:55Z

Yep, I absolutely find that diagram helpful! Now that I can see at a glance what's the hierarchy between all typeclasses, your last structure seems correct!

BTW, is this diagram accesible somewhere in the docs?

peterneyens · 2016-10-21T15:34:05Z

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

@@ -46,20 +57,45 @@ private[data] sealed abstract class ProdInstances2 extends ProdInstances3 {
 }

 private[data] sealed abstract class ProdInstances3 extends ProdInstances4 {
+  implicit def catsDataMonadForProd[F[_], G[_]](implicit FM: Monad[F], GM: Monad[G]): Monad[λ[α => Prod[F, G, α]]] = new ProdMonad[F, G] {


We can replace these λ[α => Prod[F, G, α]] by just Prod[F, G, ?].

Yep, sure!

But, what's the difference between the generated code of one and the other?

kailuowang · 2016-10-21T15:36:11Z

@pepegar I just resurrected a PR to add the hierarchy diagram to the site. #1416

peterneyens · 2016-10-21T16:06:27Z

tests/src/test/scala/cats/tests/ProdTests.scala

+
+  test("show") {
+    forAll { (l1: Option[Int], l2: Option[Int]) =>
+      val showOptionInt = implicitly[Show[Option[Int]]]


Nitpicking, but this can just be val showOptionInt = Show[Option[Int]], right ?

Yep, you're right :)

pepegar · 2016-10-24T17:25:19Z

@kailuowang If I structure the code as you suggested:

ProdInstances: MonadCombine, Order, Show ,Traverse, Contravariant
ProdInstances0: Alternative, Monad, Eq, Foldable
ProdInstances1: MonoidK, Applicative
ProdInstances2: SemigroupK, Apply
ProdInstances3: Functor

I find ambiguous implicit values for MonadCombine and Traverse:

[info] Set current project to cats (in build file:/Users/pepe/projects/scala/cats/)
[success] Total time: 2 s, completed Oct 24, 2016 4:19:14 PM
[info] Compiling 3 Scala sources to /Users/pepe/projects/scala/cats/tests/.jvm/target/scala-2.11/test-classes...
[error] /Users/pepe/projects/scala/cats/tests/src/test/scala/cats/tests/ProdTests.scala:12: ambiguous implicit values:
[error]  both method catsDataMonadCombineForProd in class ProdInstances of type [F[_], G[_]](implicit FF: cats.MonadCombine[F], implicit GF: cats.MonadCombine[G])cats.MonadCombine[[α]cats.data.Prod[F,G,α]]
[error]  and method catsDataTraverseForProd in class ProdInstances of type [F[_], G[_]](implicit FF: cats.Traverse[F], implicit GF: cats.Traverse[G])cats.Traverse[[α]cats.data.Prod[F,G,α]]
[error]  match expected type cats.functor.Invariant[[γ$0$]cats.data.Prod[Option,[+A]List[A],γ$0$]]
[error]   implicit val iso = CartesianTests.Isomorphisms.invariant[Prod[Option, List, ?]]
[error]                                                           ^
[error] /Users/pepe/projects/scala/cats/tests/src/test/scala/cats/tests/ProdTests.scala:13: could not find implicit value for parameter ev: cats.laws.discipline.CartesianTests.Isomorphisms[[α]cats.data.Prod[Option,[+A]List[A],α]]
[error]   checkAll("Prod[Option, List, Int]", CartesianTests[λ[α => Prod[Option, List, α]]].cartesian[Int, Int, Int])
[error]                                                     ^
[error] /Users/pepe/projects/scala/cats/tests/src/test/scala/cats/tests/ProdTests.scala:16: could not find implicit value for parameter iso: cats.laws.discipline.CartesianTests.Isomorphisms[[α]cats.data.Prod[Option,[+A]List[A],α]]
[error]   checkAll("Prod[Option, List, Int]", AlternativeTests[λ[α => Prod[Option, List, α]]].alternative[Int, Int, Int])
[error]                                                                                                  ^
[error] three errors found
[error] (testsJVM/test:compileIncremental) Compilation failed

What do you think on the following structure?

ProdInstances: MonadCombine, Order, Show, Contravariant
ProdInstances0: Alternative, Monad, Eq, Traverse
ProdInstances1: MonoidK, Applicative, Foldable
ProdInstances2: SemigroupK, Apply
ProdInstances3: Functor

The latter seems to work fine for me.

kailuowang · 2016-10-24T17:33:12Z

@pepegar My bad, your suggestion looks fine to me.

kailuowang · 2016-10-24T17:50:29Z

👍 on travis green

non · 2016-10-25T18:41:20Z

LGTM 👍

pepegar added 3 commits October 4, 2016 19:19

add Monad instance for cats.data.Prod

64c74fa

add missing implicit

78f4c60

create instances for Foldable and Traverse

c2c7e97

pepegar added 2 commits October 8, 2016 11:52

create instance for MonadCombine

95aed3a

create instances for Show and Order

d648c88

adelbertc reviewed Oct 13, 2016

View reviewed changes

pepegar added 2 commits October 14, 2016 14:20

check OrderLaws for Prod

2032ce0

clean up Prod.scala file

f8462cd

kailuowang suggested changes Oct 18, 2016

View reviewed changes

travisbrown mentioned this pull request Oct 18, 2016

0.8.0 time! #1412

Merged

13 tasks

make Order have more priority than Eq

4e9c489

kailuowang suggested changes Oct 19, 2016

View reviewed changes

reorder Prod instances acording to its specificity

000a583

kailuowang suggested changes Oct 20, 2016

View reviewed changes

kailuowang approved these changes Oct 20, 2016

View reviewed changes

kailuowang suggested changes Oct 20, 2016

View reviewed changes

peterneyens reviewed Oct 21, 2016

View reviewed changes

simplify test("show")

405dd93

reorder instances to allow ambiguous implicit values

867b2a2

kailuowang approved these changes Oct 24, 2016

View reviewed changes

non merged commit 5c00fdf into typelevel:master Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More instances for prod #1402

More instances for prod #1402

pepegar commented Oct 7, 2016 •

edited

Loading

codecov-io commented Oct 7, 2016 •

edited

Loading

pepegar commented Oct 13, 2016

adelbertc Oct 13, 2016

pepegar Oct 14, 2016

adelbertc Oct 13, 2016

pepegar Oct 14, 2016

adelbertc Oct 13, 2016

pepegar Oct 14, 2016

kailuowang Oct 18, 2016

pepegar Oct 19, 2016

kailuowang Oct 19, 2016

kailuowang Oct 19, 2016 •

edited

Loading

pepegar Oct 20, 2016

kailuowang Oct 20, 2016 •

edited

Loading

kailuowang left a comment •

edited

Loading

pepegar commented Oct 20, 2016

peterneyens Oct 21, 2016

pepegar Oct 24, 2016

kailuowang commented Oct 21, 2016

peterneyens Oct 21, 2016

pepegar Oct 24, 2016

pepegar commented Oct 24, 2016 •

edited

Loading

kailuowang commented Oct 24, 2016

kailuowang commented Oct 24, 2016

non commented Oct 25, 2016

More instances for prod #1402

More instances for prod #1402

Conversation

pepegar commented Oct 7, 2016 • edited Loading

codecov-io commented Oct 7, 2016 • edited Loading

Current coverage is 91.68% (diff: 100%)

pepegar commented Oct 13, 2016

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 Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

kailuowang left a comment • edited Loading

Choose a reason for hiding this comment

pepegar commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepegar commented Oct 24, 2016 • edited Loading

kailuowang commented Oct 24, 2016

kailuowang commented Oct 24, 2016

non commented Oct 25, 2016

pepegar commented Oct 7, 2016 •

edited

Loading

codecov-io commented Oct 7, 2016 •

edited

Loading

kailuowang Oct 19, 2016 •

edited

Loading

kailuowang Oct 20, 2016 •

edited

Loading

kailuowang left a comment •

edited

Loading

pepegar commented Oct 24, 2016 •

edited

Loading