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

Adding partitionEitherM #2641

Merged
merged 18 commits into from
Jan 22, 2019
3 changes: 2 additions & 1 deletion core/src/main/scala/cats/syntax/all.scala
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,5 @@ trait AllSyntaxBinCompat4
with ParallelApplySyntax
with FoldableSyntaxBinCompat0
with ReducibleSyntaxBinCompat0
with BitraverseSyntaxBinCompat0
with FoldableSyntaxBinCompat1
with BitraverseSyntaxBinCompat0
64 changes: 63 additions & 1 deletion core/src/main/scala/cats/syntax/foldable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ trait FoldableSyntaxBinCompat0 {
new FoldableOps0[F, A](fa)
}

trait FoldableSyntaxBinCompat1 {
implicit final def catsSyntaxFoldableBinCompat0[F[_]](fa: Foldable[F]): FoldableOps1[F] =
new FoldableOps1(fa)
}

final class NestedFoldableOps[F[_], G[_], A](private val fga: F[G[A]]) extends AnyVal {
def sequence_(implicit F: Foldable[F], G: Applicative[G]): G[Unit] = F.sequence_(fga)

Expand Down Expand Up @@ -195,7 +200,6 @@ final class FoldableOps[F[_], A](private val fa: F[A]) extends AnyVal {
case None ⇒ acc
}
)

}

final class FoldableOps0[F[_], A](val fa: F[A]) extends AnyVal {
Expand All @@ -214,4 +218,62 @@ final class FoldableOps0[F[_], A](val fa: F[A]) extends AnyVal {
* */
def foldMapK[G[_], B](f: A => G[B])(implicit F: Foldable[F], G: MonoidK[G]): G[B] =
F.foldMap(fa)(f)(G.algebra)

/**
* Separate this Foldable into a Tuple by an effectful separating function `A => G[Either[B, C]]`
* Equivalent to `Bitraversable#traverse` over `Alternative#separate`
*
* {{{
* scala> import cats.implicits._, cats.Eval
* scala> val list = List(1,2,3,4)
* scala> val partitioned1 = list.partitionEitherM(a => if (a % 2 == 0) Eval.now(Either.left[String, Int](a.toString)) else Eval.now(Either.right[String, Int](a)))
* Since `Eval.now` yields a lazy computation, we need to force it to inspect the result:
* scala> partitioned1.value
* res0: (List[String], List[Int]) = (List(2, 4),List(1, 3))
* scala> val partitioned2 = list.partitionEitherM(a => Eval.later(Either.right(a * 4)))
* scala> partitioned2.value
* res1: (List[Nothing], List[Int]) = (List(),List(4, 8, 12, 16))
* }}}
*/
def partitionEitherM[G[_], B, C](
f: A => G[Either[B, C]]
)(implicit A: Alternative[F], F: Foldable[F], M: Monad[G]): G[(F[B], F[C])] = {
import cats.syntax.foldable._
F.partitionEitherM[G, A, B, C](fa)(f)(A, M)
}
}

final class FoldableOps1[F[_]](private val F: Foldable[F]) extends AnyVal {

/**
* Separate this Foldable into a Tuple by an effectful separating function `A => G[Either[B, C]]`
* Equivalent to `Bitraversable#traverse` over `Alternative#separate`
*
* {{{
* scala> import cats.implicits._, cats.Foldable, cats.Eval
* scala> val list = List(1,2,3,4)
* scala> val partitioned1 = Foldable[List].partitionEitherM(list)(a => if (a % 2 == 0) Eval.now(Either.left[String, Int](a.toString)) else Eval.now(Either.right[String, Int](a)))
* Since `Eval.now` yields a lazy computation, we need to force it to inspect the result:
* scala> partitioned1.value
* res0: (List[String], List[Int]) = (List(2, 4),List(1, 3))
* scala> val partitioned2 = Foldable[List].partitionEitherM(list)(a => Eval.later(Either.right(a * 4)))
* scala> partitioned2.value
* res1: (List[Nothing], List[Int]) = (List(),List(4, 8, 12, 16))
* }}}
*/
def partitionEitherM[G[_], A, B, C](fa: F[A])(f: A => G[Either[B, C]])(implicit A: Alternative[F],
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicated in two places. What if instead we added a partitionEitherM method to the Foldable companion object and had the FoldableOps0 method delegate to it? At that point, I'm not sure if it makes sense to have the FoldableOps1 extension to the type class itself; people could just call the method on the companion object if they wanted it. WDYT @kailuowang and @blast-hardcheese?

Sorry I'm sure that this is annoying. I think that Cats maintainers need to put some effort into our approach to binary compatibility and making it easier on other contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidating definitely felt like the right approach, but doing the same syntax lookup again while we're already working around bin compat seemed worse. I can make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to generalize this to any Bifoldable instead of just Either?

  def partitionBiffyM[G[_], H[_,_], A, B, C](fa: F[A])(f: A => G[H[B, C]])(implicit A: Alternative[F],
                                                                         M: Monad[G], H: Bifoldable[H]): G[(F[B], F[C])] = {
    import cats.instances.tuple._

    implicit val mb: Monoid[F[B]] = A.algebra[B]
    implicit val mc: Monoid[F[C]] = A.algebra[C]

    F.foldMapM[G, A, (F[B], F[C])](fa)(
      a =>
        M.map(f(a)){
          H.bifoldMap[B, C, (F[B], F[C])](_)(
            b => (A.pure(b), A.empty[C]),
            c => (A.empty[B], A.pure(c)))
      }
    )
  }

Then you could use it for the partitionEitherM use-case, but you could also use it for other Bifoldables like Tuple2:

scala> Foldable[List].partitionBiffyM(list)(a => Eval.now((a - 1, a + 1)))
res5: cats.Eval[(List[Int], List[Int])] = cats.Eval$$anon$6@67bef13e

scala> res5.value
res6: (List[Int], List[Int]) = (List(0, 1, 2, 3),List(2, 3, 4, 5))

I'm hoping that someone else has a better name than mine 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to suggest a broader refactor that includes partitionEither. I can generalize to partitionBiffy as well as partitionBiffyM, while providing the concretized partitionEitherM that just defers to partitionBiffyM, if that is desirable

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 gave it a shot, though the examples are getting increasingly convoluted I think.

M: Monad[G]): G[(F[B], F[C])] = {
import cats.instances.tuple._

implicit val mb: Monoid[F[B]] = A.algebra[B]
implicit val mc: Monoid[F[C]] = A.algebra[C]

F.foldMapM[G, A, (F[B], F[C])](fa)(
a =>
M.map(f(a)) {
case Right(c) => (A.empty[B], A.pure(c))
case Left(b) => (A.pure(b), A.empty[C])
}
)
}
}
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/syntax/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ package object syntax {
object either extends EitherSyntax with EitherSyntaxBinCompat0
object eq extends EqSyntax
object flatMap extends FlatMapSyntax
object foldable extends FoldableSyntax with FoldableSyntaxBinCompat0
object foldable extends FoldableSyntax with FoldableSyntaxBinCompat0 with FoldableSyntaxBinCompat1
object functor extends FunctorSyntax
object functorFilter extends FunctorFilterSyntax
object group extends GroupSyntax
Expand Down
57 changes: 57 additions & 0 deletions tests/src/test/scala/cats/tests/FoldableSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,63 @@ abstract class FoldableSuite[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
}
}

test("Foldable#partitionEitherM retains size") {
import cats.syntax.foldable._
forAll { (fi: F[Int], f: Int => Either[String, String]) =>
val vector = Foldable[F].toList(fi).toVector
val result = Foldable[Vector].partitionEitherM(vector)(f.andThen(Option.apply)).map {
case (lefts, rights) =>
(lefts <+> rights).size
}
result should ===(Option(vector.size))
}
}

test("Foldable#partitionEitherM consistent with List#partition") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh this is a good idea for a test 👍

import cats.syntax.foldable._
forAll { (fi: F[Int], f: Int => Either[String, String]) =>
val list = Foldable[F].toList(fi)
val partitioned = Foldable[List].partitionEitherM(list)(f.andThen(Option.apply))
val (ls, rs) = list
.map(f)
.partition({
case Left(_) => true
case Right(_) => false
})

partitioned.map(_._1.map(_.asLeft[String])) should ===(Option(ls))
partitioned.map(_._2.map(_.asRight[String])) should ===(Option(rs))
}
}

test("Foldable#partitionEitherM to one side is identity") {
import cats.syntax.foldable._
forAll { (fi: F[Int], f: Int => String) =>
val list = Foldable[F].toList(fi)
val g: Int => Option[Either[Double, String]] = f.andThen(Right.apply).andThen(Option.apply)
val h: Int => Option[Either[String, Double]] = f.andThen(Left.apply).andThen(Option.apply)

val withG = Foldable[List].partitionEitherM(list)(g).map(_._2)
withG should ===(Option(list.map(f)))

val withH = Foldable[List].partitionEitherM(list)(h).map(_._1)
withH should ===(Option(list.map(f)))
}
}

test("Foldable#partitionEitherM remains sorted") {
import cats.syntax.foldable._
forAll { (fi: F[Int], f: Int => Either[String, String]) =>
val list = Foldable[F].toList(fi)

val sorted = list.map(f).sorted
val pairs = Foldable[List].partitionEitherM(sorted)(Option.apply)

pairs.map(_._1.sorted) should ===(pairs.map(_._1))
pairs.map(_._2.sorted) should ===(pairs.map(_._2))
}
}

test(s"Foldable[$name] summation") {
forAll { (fa: F[Int]) =>
val total = iterator(fa).sum
Expand Down