-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add distributive typeclass and some instances #2046
Conversation
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]] = /* ... */
} |
I think we can also do instances for Tuple2K :) |
@@ -1,6 +1,14 @@ | |||
package cats | |||
|
|||
|
|||
private [cats] trait ComposedDistributive[F[_], G[_]] extends Distributive[λ[α => F[G[α]]]] with ComposedFunctor[F, G] { outer => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scalastyle doesn't like the space before [cats]
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
} | ||
|
||
// Add syntax to functor as part of importing distributive syntax. | ||
final class DistributiveOps[F[_]: Functor, A](val fa: F[A]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this an AnyVal? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the class has the Functor constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you're right! We should move the Functor
constraint to the method instead
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you're right, disregard me! :D
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
@LukaJCB I think everything should be in there, now. Let me know if you have problems with anything. |
Hey Colt, looks really great, thanks again! I think it's ready to merge, but you'll need to add some MiMa Exceptions :) |
implicit def catsDataFunctorForKleisli[F[_], A](implicit F0: Functor[F]): Functor[Kleisli[F, A, ?]] = | ||
new KleisliFunctor[F, A] { def F: Functor[F] = F0 } | ||
} | ||
|
||
private[data] sealed abstract class KleisliInstances8 { | ||
implicit def kleisliDistributive[F[_], R](implicit F0: Distributive[F]): Distributive[Kleisli[F, R, ?]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
|
||
implicit def catsDataFunctorForTuple2K[F[_], G[_]](implicit FF: Functor[F], GG: Functor[G]): Functor[λ[α => Tuple2K[F, G, α]]] = new Tuple2KFunctor[F, G] { | ||
def F: Functor[F] = FF | ||
def G: Functor[G] = GG | ||
} | ||
} | ||
|
||
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] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in Kleisli
, need to swap the two instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
@@ -167,6 +167,11 @@ private[data] sealed abstract class NestedInstances12 { | |||
new NestedInvariant[F, G] { | |||
val FG: Invariant[λ[α => F[G[α]]]] = Invariant[F].composeContravariant[G] | |||
} | |||
|
|||
implicit def catsDataDistributiveForNested[F[_]: Distributive, G[_]: Distributive]: Distributive[Nested[F, G, ?]] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to go above the Functor
instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
private[instances] sealed trait Function1Instances0 { | ||
implicit def catsStdContravariantForFunction1[R]: Contravariant[? => R] = | ||
new Contravariant[? => R] { | ||
def contramap[T1, T0](fa: T1 => R)(f: T0 => T1): T0 => R = | ||
fa.compose(f) | ||
} | ||
|
||
implicit def functior1Distributive[T1]: Distributive[T1 => ?] = new Distributive[T1 => ?] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functior1Distributive
-> catsStdDistributiveForFunction1
@@ -113,6 +113,11 @@ class KleisliSuite extends CatsSuite { | |||
checkAll("Functor[Kleisli[Option, Int, ?]]", SerializableTests.serializable(Functor[Kleisli[Option, Int, ?]])) | |||
} | |||
|
|||
{ | |||
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, ?]])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It should be Distributive[Kleisli[Function0, Int, ?]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
|
||
{ | ||
//Distributive composition | ||
checkAll("Nested[Function0, Function0, ?]", DistributiveTests[Nested[Function0, Function0, ?]].distributive[Int, Int, Int, Option, Function0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should test composition of two different data types like the Serializable test below, i.e. Nested[Function1[Int, ?], Function0, ?]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
scalastyle
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
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 theDistributive
instance.We want to add
distribute[G[_]: Distributive, A, B](f: A => G[B]): G[F[B]]
to the functorF
. This means that we should be able to turnList[Int => Int]
intoInt => List[Int]
, but I don't know how to add the syntax to the Functor that's passed to it.