Skip to content
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

Merged
merged 14 commits into from
Dec 12, 2017

Conversation

coltfred
Copy link
Contributor

@coltfred coltfred commented Nov 26, 2017

Things still left:

  • Laws and Tests
    • Function0 Test
    • Function1 Test
    • Nested Test
    • Kleisli Test
    • Tuple2K Test
  • Syntax that makes sense
  • More instances?

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
Copy link
Member

LukaJCB commented Nov 26, 2017

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 LukaJCB mentioned this pull request Nov 26, 2017
3 tasks
@LukaJCB
Copy link
Member

LukaJCB commented Nov 26, 2017

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 =>
Copy link
Member

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.

Copy link
Contributor Author

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]) {
Copy link
Member

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? :)

Copy link
Contributor Author

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.

Copy link
Member

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)
Copy link
Member

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)
}

Copy link
Contributor Author

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?

Copy link
Member

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-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

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
Copy link
Contributor Author

coltfred commented Dec 9, 2017

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

@LukaJCB
Copy link
Member

LukaJCB commented Dec 10, 2017

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

LukaJCB
LukaJCB previously approved these changes Dec 10, 2017
@kailuowang kailuowang added this to the 1.0.0 milestone Dec 11, 2017
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, ?]] =
Copy link
Contributor

@kailuowang kailuowang Dec 11, 2017

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

Copy link
Contributor Author

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] {
Copy link
Contributor

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.

Copy link
Contributor Author

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, ?]] =
Copy link
Contributor

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

Copy link
Contributor Author

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 => ?] {
Copy link
Contributor

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, ?]]))
Copy link
Contributor

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, ?]]

Copy link
Contributor Author

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])
Copy link
Contributor

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, ?]

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@kailuowang
Copy link
Contributor

scalastyle

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

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Distributive typeclass
4 participants