From 568daf4c75b9261824f00e72fefce87cd27ddc5e Mon Sep 17 00:00:00 2001 From: Luka Jacobowitz Date: Sun, 2 Sep 2018 20:13:47 +0200 Subject: [PATCH] Address feedback --- core/src/main/scala/cats/data/EitherT.scala | 47 +------------------ core/src/main/scala/cats/data/OptionT.scala | 41 ++++++++++++---- .../main/scala/cats/instances/option.scala | 4 +- .../test/scala/cats/tests/EitherTSuite.scala | 22 --------- .../test/scala/cats/tests/OptionTSuite.scala | 23 +++++++++ 5 files changed, 58 insertions(+), 79 deletions(-) diff --git a/core/src/main/scala/cats/data/EitherT.scala b/core/src/main/scala/cats/data/EitherT.scala index 86ba284edd7..82ff7e1ac50 100644 --- a/core/src/main/scala/cats/data/EitherT.scala +++ b/core/src/main/scala/cats/data/EitherT.scala @@ -3,7 +3,6 @@ package data import cats.Bifunctor import cats.instances.either._ -import cats.instances.option._ import cats.syntax.either._ /** @@ -516,27 +515,7 @@ private[data] abstract class EitherTInstances extends EitherTInstances1 { EitherT(F.defer(fa.value)) } - implicit def catsDataTraverseFilterForEitherT[F[_], L](implicit F0: TraverseFilter[F]): TraverseFilter[EitherT[F, L, ?]] = - new EitherTFunctorFilter[F, L] with TraverseFilter[EitherT[F, L, ?]] { - implicit def F: FunctorFilter[F] = F0 - def traverse: Traverse[EitherT[F, L, ?]] = catsDataTraverseForEitherT[F, L](F0.traverse) - - def traverseFilter[G[_], A, B] - (fa: EitherT[F, L, A]) - (f: A => G[Option[B]]) - (implicit G: Applicative[G]): G[EitherT[F, L, B]] = - G.map( - F0.traverseFilter[G, Either[L, A], Either[L, B]](fa.value) { - case l@Left(_) => G.pure(Option(l.rightCast[B])) - case Right(a) => G.map(f(a))(_.map(Either.right)) - })(EitherT(_)) - - override def filterA[G[_], A] - (fa: EitherT[F, L, A]) - (f: A => G[Boolean]) - (implicit G: Applicative[G]): G[EitherT[F, L, A]] = - G.map(F0.filterA(fa.value)(_.fold(_ => G.pure(true), f)))(EitherT[F, L, A]) - } + } private[data] abstract class EitherTInstances1 extends EitherTInstances2 { @@ -568,9 +547,6 @@ private[data] abstract class EitherTInstances1 extends EitherTInstances2 { override def ensureOr[A](fa: EitherT[F, L, A])(error: (A) => L)(predicate: (A) => Boolean): EitherT[F, L, A] = fa.ensureOr(error)(predicate)(F) } - - implicit def catsDataFunctorFilterForEitherT[F[_], L](implicit F0: FunctorFilter[F]): FunctorFilter[EitherT[F, L, ?]] = - new EitherTFunctorFilter[F, L] { implicit def F = F0 } } private[data] abstract class EitherTInstances2 extends EitherTInstances3 { @@ -730,24 +706,3 @@ private[data] sealed trait EitherTOrder[F[_], L, A] extends Order[EitherT[F, L, override def compare(x: EitherT[F, L, A], y: EitherT[F, L, A]): Int = x compare y } - -private[data] sealed trait EitherTFunctorFilter[F[_], E] extends FunctorFilter[EitherT[F, E, ?]] { - implicit def F: FunctorFilter[F] - - override def functor: Functor[EitherT[F, E, ?]] = EitherT.catsDataFunctorForEitherT[F, E](F.functor) - - def mapFilter[A, B](fa: EitherT[F, E, A])(f: (A) => Option[B]): EitherT[F, E, B] = - EitherT[F, E, B](F.mapFilter(fa.value)(_.traverse(f))) - - override def collect[A, B](fa: EitherT[F, E, A])(f: PartialFunction[A, B]): EitherT[F, E, B] = { - EitherT[F, E, B](F.mapFilter(fa.value)(_.traverse(f.lift))) - } - - override def flattenOption[A](fa: EitherT[F, E, Option[A]]): EitherT[F, E, A] = { - EitherT[F, E, A](F.flattenOption[Either[E, A]](F.functor.map(fa.value)(Traverse[Either[E, ?]].sequence[Option, A]))) - } - - override def filter[A](fa: EitherT[F, E, A])(f: (A) => Boolean): EitherT[F, E, A] = { - EitherT[F, E, A](F.filter(fa.value)(_.forall(f))) - } -} diff --git a/core/src/main/scala/cats/data/OptionT.scala b/core/src/main/scala/cats/data/OptionT.scala index 9e057eec660..9cac0f5c2a5 100644 --- a/core/src/main/scala/cats/data/OptionT.scala +++ b/core/src/main/scala/cats/data/OptionT.scala @@ -1,7 +1,7 @@ package cats package data -import cats.instances.option.{catsStdInstancesForOption => optionInstance} +import cats.instances.option.{catsStdInstancesForOption => optionInstance, catsStdTraverseFilterForOption} import cats.syntax.either._ /** @@ -235,19 +235,25 @@ private[data] sealed abstract class OptionTInstances extends OptionTInstances0 { OptionT(F.defer(fa.value)) } - implicit def optionTFunctorFilter[F[_]: Functor]: FunctorFilter[OptionT[F, ?]] = { - new FunctorFilter[OptionT[F, ?]] { - override val functor: Functor[OptionT[F, ?]] = OptionT.catsDataFunctorForOptionT[F] + implicit def catsDateTraverseFilterForOptionT[F[_]](implicit F0: Traverse[F]): TraverseFilter[OptionT[F, ?]] = + new OptionTFunctorFilter[F] with TraverseFilter[OptionT[F, ?]] { + implicit def F: Functor[F] = F0 - override def mapFilter[A, B](fa: OptionT[F, A])(f: (A) => Option[B]): OptionT[F, B] = fa.subflatMap(f) + val traverse: Traverse[OptionT[F, ?]] = OptionT.catsDataTraverseForOptionT[F] - override def collect[A, B](fa: OptionT[F, A])(f: PartialFunction[A, B]): OptionT[F, B] = fa.subflatMap(f.lift) + def traverseFilter[G[_], A, B](fa: OptionT[F, A]) + (f: A => G[Option[B]]) + (implicit G: Applicative[G]): G[OptionT[F, B]] = + G.map(Traverse[F].traverse[G, Option[A], Option[B]](fa.value) { + oa => TraverseFilter[Option].traverseFilter(oa)(f) + })(OptionT[F, B]) - override def flattenOption[A](fa: OptionT[F, Option[A]]): OptionT[F, A] = fa.subflatMap(identity) + override def filterA[G[_], A](fa: OptionT[F, A]) + (f: A => G[Boolean]) + (implicit G: Applicative[G]): G[OptionT[F, A]] = + G.map(Traverse[F].traverse(fa.value)(TraverseFilter[Option].filterA[G, A](_)(f)))(OptionT[F, A]) - override def filter[A](fa: OptionT[F, A])(f: (A) => Boolean): OptionT[F, A] = fa.filter(f) } - } } private[data] sealed abstract class OptionTInstances0 extends OptionTInstances1 { @@ -265,6 +271,9 @@ private[data] sealed abstract class OptionTInstances0 extends OptionTInstances1 implicit def catsDataPartialOrderForOptionT[F[_], A](implicit F0: PartialOrder[F[Option[A]]]): PartialOrder[OptionT[F, A]] = new OptionTPartialOrder[F, A] { implicit val F = F0 } + + implicit def catsDateFunctorFilterForOptionT[F[_]](implicit F0: Functor[F]): FunctorFilter[OptionT[F, ?]] = + new OptionTFunctorFilter[F] { implicit val F = F0 } } private[data] sealed abstract class OptionTInstances1 extends OptionTInstances2 { @@ -401,6 +410,20 @@ private[data] sealed trait OptionTPartialOrder[F[_], A] extends PartialOrder[Opt override def partialCompare(x: OptionT[F, A], y: OptionT[F, A]): Double = x partialCompare y } +private[data] sealed trait OptionTFunctorFilter[F[_]] extends FunctorFilter[OptionT[F, ?]] { + implicit def F: Functor[F] + + def functor: Functor[OptionT[F, ?]] = OptionT.catsDataFunctorForOptionT[F] + + def mapFilter[A, B](fa: OptionT[F, A])(f: (A) => Option[B]): OptionT[F, B] = fa.subflatMap(f) + + override def collect[A, B](fa: OptionT[F, A])(f: PartialFunction[A, B]): OptionT[F, B] = fa.subflatMap(f.lift) + + override def flattenOption[A](fa: OptionT[F, Option[A]]): OptionT[F, A] = fa.subflatMap(identity) + + override def filter[A](fa: OptionT[F, A])(f: (A) => Boolean): OptionT[F, A] = fa.filter(f) +} + private[data] sealed trait OptionTOrder[F[_], A] extends Order[OptionT[F, A]] with OptionTPartialOrder[F, A]{ override implicit def F: Order[F[Option[A]]] diff --git a/core/src/main/scala/cats/instances/option.scala b/core/src/main/scala/cats/instances/option.scala index 16d72006b0a..0a84a02a655 100644 --- a/core/src/main/scala/cats/instances/option.scala +++ b/core/src/main/scala/cats/instances/option.scala @@ -136,13 +136,13 @@ trait OptionInstancesBinCompat0 { def traverseFilter[G[_], A, B](fa: Option[A])(f: (A) => G[Option[B]])(implicit G: Applicative[G]): G[Option[B]] = fa match { - case _: None.type => G.pure(Option.empty[B]) + case None => G.pure(Option.empty[B]) case Some(a) => f(a) } override def filterA[G[_], A](fa: Option[A])(f: (A) => G[Boolean])(implicit G: Applicative[G]): G[Option[A]] = fa match { - case _: None.type => G.pure(Option.empty[A]) + case None => G.pure(Option.empty[A]) case Some(a) => G.map(f(a))(b => if (b) Some(a) else None) } diff --git a/tests/src/test/scala/cats/tests/EitherTSuite.scala b/tests/src/test/scala/cats/tests/EitherTSuite.scala index 883e5115b4d..e87400d1c97 100644 --- a/tests/src/test/scala/cats/tests/EitherTSuite.scala +++ b/tests/src/test/scala/cats/tests/EitherTSuite.scala @@ -47,28 +47,6 @@ class EitherTSuite extends CatsSuite { } - { - //If a TraverseFilter for F is defined - implicit val F = ListWrapper.traverseFilter - - checkAll("EitherT[ListWrapper, Int, ?]", - TraverseFilterTests[EitherT[ListWrapper, Int, ?]].traverseFilter[Int, Int, Int]) - checkAll("TraverseFilter[EitherT[ListWrapper, Int, ?]]", - SerializableTests.serializable(TraverseFilter[EitherT[ListWrapper, Int, ?]])) - - } - - { - //If a functorFilter for F is defined - implicit val F = ListWrapper.functorFilter - - checkAll("EitherT[ListWrapper, Int, ?]", - FunctorFilterTests[EitherT[ListWrapper, Int, ?]].functorFilter[Int, Int, Int]) - checkAll("FunctorFilter[EitherT[ListWrapper, Int, ?]]", - SerializableTests.serializable(FunctorFilter[EitherT[ListWrapper, Int, ?]])) - - } - { //if a Monad is defined diff --git a/tests/src/test/scala/cats/tests/OptionTSuite.scala b/tests/src/test/scala/cats/tests/OptionTSuite.scala index bf34daa456f..c8df8136dd9 100644 --- a/tests/src/test/scala/cats/tests/OptionTSuite.scala +++ b/tests/src/test/scala/cats/tests/OptionTSuite.scala @@ -13,6 +13,29 @@ class OptionTSuite extends CatsSuite { checkAll("OptionT[Eval, ?]", FunctorFilterTests[OptionT[Eval, ?]].functorFilter[Int, Int, Int]) + { + //If a Functor for F is defined + implicit val F = ListWrapper.functor + + checkAll("OptionT[ListWrapper, ?]", + FunctorFilterTests[OptionT[ListWrapper, ?]].functorFilter[Int, Int, Int]) + checkAll("FunctorFilter[OptionT[ListWrapper, ?]]", + SerializableTests.serializable(FunctorFilter[OptionT[ListWrapper, ?]])) + + } + + + { + //If a Traverse for F is defined + implicit val F = ListWrapper.traverse + + checkAll("OptionT[ListWrapper, ?]", + TraverseFilterTests[OptionT[ListWrapper, ?]].traverseFilter[Int, Int, Int]) + checkAll("TraverseFilter[OptionT[ListWrapper, ?]]", + SerializableTests.serializable(TraverseFilter[OptionT[ListWrapper, ?]])) + + } + { implicit val F = ListWrapper.eqv[Option[Int]]