Add distributive typeclass and some instances by coltfred · Pull Request #2046 · typelevel/cats

coltfred · 2017-11-26T03:53:16Z

Started working on this to fix Add Distributive typeclass #2014

Things still left:

I started on it this weekend and wanted to push it up to start getting feedback as well as help on the syntax. Distributive is weird in that it's adding syntax to the Functor, not to the type that has the Distributive instance.

We want to add distribute[G[_]: Distributive, A, B](f: A => G[B]): G[F[B]] to the functor F. This means that we should be able to turn List[Int => Int] into Int => List[Int], but I don't know how to add the syntax to the Functor that's passed to it.

LukaJCB · 2017-11-26T22:36:58Z

Hey Colt, for the syntax you can do something like this:

trait DistributiveSyntax extends Distributive.ToDistributiveOps {
  implicit final def catsSyntaxDistributiveOps[F[_]: Functor, A](fa: F[A]): DistributiveOps[F, 
 A] =
    new DistributiveOps[F, A](fa)
}

final class DistributiveOps[F[_]: Functor, A](val fa: F[A]) extends AnyVal {
  def distribute[G[_]: Distributive, B](f: A => G[B]): G[F[B]] = /* ... */
}

LukaJCB · 2017-11-26T23:20:38Z

I think we can also do instances for Tuple2K :)

LukaJCB · 2017-11-27T23:44:27Z

 package cats


+private [cats] trait ComposedDistributive[F[_], G[_]] extends Distributive[λ[α => F[G[α]]]] with ComposedFunctor[F, G] { outer =>


Scalastyle doesn't like the space before [cats] here.

LukaJCB · 2017-11-28T08:44:50Z

+}
+
+// Add syntax to functor as part of importing distributive syntax.
+final class DistributiveOps[F[_]: Functor, A](val fa: F[A]) {


Can we make this an AnyVal? :)

No, because the class has the Functor constraint.

Ah yes, you're right! We should move the Functor constraint to the method instead

LukaJCB · 2017-11-28T08:47:16Z

+import cats.evidence.===
+
+trait DistributiveSyntax extends Distributive.ToDistributiveOps {
+  implicit final def catsSyntaxDistributiveOps[F[_]: Functor, A](fa: F[A]): DistributiveOps[F, A] = new DistributiveOps[F, A](fa)


Maybe we should add another class for cosequence, we wouldn't need cats.evidence then:

final class CosequenceOps[F[_]: Functor, G[_]: Distributive, A](val fga: F[G[A]]) extends AnyVal { def cosequence: G[F[A]] = G.cosequence(fga) }

Hmmm, not sure I see a reason why that's better? The evidence constraint is on just the one method and it saves the extra class for syntax. Is there a reason it should be preferred?

Maybe you're right, disregard me! :D

codecov-io · 2017-11-29T03:17:51Z

Codecov Report

Merging #2046 into master will increase coverage by 0.02%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master    #2046      +/-   ##
==========================================
+ Coverage   94.63%   94.65%   +0.02%     
==========================================
  Files         318      322       +4     
  Lines        5391     5449      +58     
  Branches      209      215       +6     
==========================================
+ Hits         5102     5158      +56     
- Misses        289      291       +2

Impacted Files	Coverage Δ
...scala/cats/laws/discipline/DistributiveTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/Composed.scala	`96.42% <100%> (+0.13%)`	⬆️
core/src/main/scala/cats/instances/function.scala	`93.75% <100%> (+1.44%)`	⬆️
core/src/main/scala/cats/data/Tuple2K.scala	`95.34% <100%> (+0.34%)`	⬆️
core/src/main/scala/cats/package.scala	`100% <100%> (ø)`	⬆️
...ws/src/main/scala/cats/laws/DistributiveLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/Nested.scala	`95.45% <100%> (+0.21%)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`97.77% <100%> (+0.07%)`	⬆️
core/src/main/scala/cats/Distributive.scala	`66.66% <66.66%> (ø)`
...rc/main/scala/cats/syntax/DistributiveSyntax.scala	`66.66% <66.66%> (ø)`
... and 5 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 8bb5c01...b686896. Read the comment docs.

coltfred · 2017-11-29T03:19:36Z

Laws and tests are all that's needed. Sorry I haven't been able to hammer those out it's been a busy week. I'll try and get the laws written tomorrow.

coltfred · 2017-12-09T18:34:22Z

@LukaJCB I think everything should be in there, now. Let me know if you have problems with anything.

LukaJCB · 2017-12-10T16:08:53Z

Hey Colt, looks really great, thanks again! I think it's ready to merge, but you'll need to add some MiMa Exceptions :)

kailuowang · 2017-12-11T20:24:22Z

 }

+private[data] sealed abstract class KleisliInstances8 {
+  implicit def kleisliDistributive[F[_], R](implicit F0: Distributive[F]): Distributive[Kleisli[F, R, ?]] =


The Cats convention is that instances that are more specific should have higher priority. So we should probably swap the Functor and Distributive instance here.
Also, according to naming convention of implicits, it should be catsDataDistributiveForKleisli

kailuowang · 2017-12-11T20:29:15Z


+private[data] sealed abstract class Tuple2KInstances8 {
+
+  implicit def catsDataDistributiveForTuple2K[F[_], G[_]](implicit FF: Distributive[F], GG: Distributive[G]): Distributive[λ[α => Tuple2K[F, G, α]]] = new Tuple2KDistributive[F, G] {


Same as in Kleisli, need to swap the two instances.

kailuowang · 2017-12-11T20:34:32Z

      val FG: Invariant[λ[α => F[G[α]]]] = Invariant[F].composeContravariant[G]
    }
+
+  implicit def catsDataDistributiveForNested[F[_]: Distributive, G[_]: Distributive]: Distributive[Nested[F, G, ?]] =


It needs to go above the Functor instance

kailuowang · 2017-12-11T20:40:50Z

        fa.compose(f)
    }
+
+  implicit def functior1Distributive[T1]: Distributive[T1 => ?] = new Distributive[T1 => ?] {


functior1Distributive -> catsStdDistributiveForFunction1

kailuowang · 2017-12-11T21:07:31Z


+  {
+    checkAll("Kleisli[Function0, Int, ?]", DistributiveTests[Kleisli[Function0, Int, ?]].distributive[Int, Int, Int, Option, Id])
+    checkAll("Distributive[Kleisli[Option, Int, ?]]", SerializableTests.serializable(Distributive[Kleisli[Function0, Int, ?]]))


Nit: It should be Distributive[Kleisli[Function0, Int, ?]]

kailuowang · 2017-12-11T21:12:10Z

+
+  {
+    //Distributive composition
+    checkAll("Nested[Function0, Function0, ?]", DistributiveTests[Nested[Function0, Function0, ?]].distributive[Int, Int, Int, Option, Function0])


I wonder if we should test composition of two different data types like the Serializable test below, i.e. Nested[Function1[Int, ?], Function0, ?]

I ran into an error about Eq instances because distributive in the Tests needs to be able to compare. I gave up and thought this was a reasonable compromise.

Ah, that's due to the Eq instance for Function1 isn't provided by default
You can do the following

import cats.laws.discipline.eq._ //Distributive composition checkAll("Nested[Function1[Int, ?], Function0, ?]", DistributiveTests[Nested[Function1[Int, ?], Function0, ?]].distributive[Int, Int, Int, Option, Function0])

That passes.

kailuowang · 2017-12-12T01:40:53Z

scalastyle

/home/travis/build/typelevel/cats/core/src/main/scala/cats/data/Nested.scala:142:0: Whitespace at end of line

LukaJCB

Thanks again!

Add distributive typeclass and some instances

a87b82b

LukaJCB mentioned this pull request Nov 26, 2017

Get ready for RC2 #2044

Merged

3 tasks

Add syntax to functor + distributive

30312b1

LukaJCB reviewed Nov 27, 2017

View reviewed changes

Scalastyle

5d14de2

LukaJCB reviewed Nov 28, 2017

View reviewed changes

add Tuple2K instance

1f3962c

coltfred added 5 commits November 30, 2017 17:34

Move Functor requirement off the class

76915e6

WIP laws cosequence and composition

0735388

Add rest of the tests

87db30d

Merge branch 'master' into addDistributive

8499505

tuple2k test

d646d83

Merge branch 'master' into addDistributive

85d24a2

Add Mima exceptions

fc59e26

LukaJCB previously approved these changes Dec 10, 2017

View reviewed changes

kailuowang added this to the 1.0.0 milestone Dec 11, 2017

kailuowang added the enhancement label Dec 11, 2017

kailuowang reviewed Dec 11, 2017

View reviewed changes

address code review

b5daed3

coltfred dismissed LukaJCB’s stale review via b5daed3 December 12, 2017 00:47

coltfred and others added 2 commits December 11, 2017 20:39

Expand test and fix scalastyle

7a2408a

add more mima exceptions

b686896

kailuowang approved these changes Dec 12, 2017

View reviewed changes

LukaJCB reviewed Dec 12, 2017

View reviewed changes

LukaJCB merged commit b91e9bb into typelevel:master Dec 12, 2017

lemastero mentioned this pull request Jan 10, 2019

Distributive members are swapped compared to Haskell/Purescript scalaz/scalaz#2034

Open

		package cats


		private [cats] trait ComposedDistributive[F[_], G[_]] extends Distributive[λ[α => F[G[α]]]] with ComposedFunctor[F, G] { outer =>


		private[data] sealed abstract class Tuple2KInstances8 {

		implicit def catsDataDistributiveForTuple2K[F[_], G[_]](implicit FF: Distributive[F], GG: Distributive[G]): Distributive[λ[α => Tuple2K[F, G, α]]] = new Tuple2KDistributive[F, G] {

Uh oh!

Uh oh!

Conversation

coltfred commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaJCB commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaJCB commented Nov 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coltfred commented Nov 29, 2017

Uh oh!

coltfred commented Dec 9, 2017

Uh oh!

LukaJCB commented Dec 10, 2017

Uh oh!

kailuowang Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Dec 12, 2017

Uh oh!

LukaJCB left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coltfred commented Nov 26, 2017 •

edited

Loading

LukaJCB commented Nov 26, 2017 •

edited

Loading

codecov-io commented Nov 29, 2017 •

edited

Loading

kailuowang Dec 11, 2017 •

edited

Loading