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.

Sign up for GitHub

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

Jump to bottom

Add combineAllOption to Foldable #2380

Merged
merged 11 commits into from
Nov 14, 2019
Merged

Add combineAllOption to Foldable #2380

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e3bd620
Added combineAllOption to Foldable
barambani Aug 10, 2018
94fcff1
Attempt
barambani Sep 30, 2018
7f252c7
Merge branch 'master' into issue-2363
barambani May 8, 2019
7f8e044
prePR
barambani May 8, 2019
200d9c0
Merge branch 'master' into issue-2363
barambani Nov 8, 2019
654aae5
Added overrides for iterator, adapted tests, deprecated Stream
barambani Nov 8, 2019
64053b5
Removed tc iterator and changed with LazyList for 2.13 and Stream for
barambani Nov 9, 2019
427c7f5
Restored original iterator in tests, added iterable syntax and test
barambani Nov 9, 2019
38cbe68
Removed `override` non needed, refined tests
barambani Nov 9, 2019
f951eef
Implemented fold in terms of iterable
barambani Nov 9, 2019
2d835f1
Changed to Iterable
barambani Nov 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions core/src/main/scala-2.12/cats/compat/FoldableCompat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package cats
package compat

private[cats] trait FoldableCompat[F[_]] { this: Foldable[F] =>
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is private[cats] can people override outside of cats? I don't know how this works when it is mixed into Foldble. Is it even publicly visible at all?

Copy link
Contributor Author

@barambani barambani Nov 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is not accessible outside cats so FoldableCompat cannot be extended but iterable can be overridden through Foldable.
I ran a quick test for this extending from outside the cats package with

object test {

  trait T1[F[_]] extends cats.Foldable[F] {
    override def iterable[A](fa: F[A]): Stream[A] = ???
  }

  trait T2[F[_]] extends cats.compat.FoldableCompat[F] {
    override def iterable[A](fa: F[A]): Stream[A] = ???
  }
}

and it seems that T1 is ok and T2 fails as expected on 2.12 and 2.13

[error] /Users/fmariotti/open-source/cats/core/src/main/scala/test.scala:7:38: trait FoldableCompat in package compat cannot be accessed in package cats.compat
[error]   trait T2[F[_]] extends cats.compat.FoldableCompat[F] {
[error]                                      ^
[error] one error found
[error] (coreJVM / Compile / compileIncremental) Compilation failed


def iterable[A](fa: F[A]): Stream[A] =
foldRight[A, Stream[A]](fa, Eval.now(Stream.empty)) { (a, eb) =>
eb.map(Stream.cons(a, _))
}.value
}

private[cats] object FoldableCompat {

trait ToFoldableCompatOps {
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 couldn't make simulacrum generate the syntax for this so I added it manually.

implicit final def foldableCompatSyntax[F[_], A](fa: F[A]): FoldableCompatAllOps[F, A] =
new FoldableCompatAllOps(fa)
}

final class FoldableCompatAllOps[F[_], A](private val fa: F[A]) extends AnyVal {
def iterable(implicit F: Foldable[F]): Stream[A] = F.iterable(fa)
}
}
2 changes: 2 additions & 0 deletions core/src/main/scala-2.12/cats/instances/stream.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances {

override def toList[A](fa: Stream[A]): List[A] = fa.toList

override def iterable[A](fa: Stream[A]): Stream[A] = fa

override def reduceLeftOption[A](fa: Stream[A])(f: (A, A) => A): Option[A] =
fa.reduceLeftOption(f)

Expand Down
22 changes: 22 additions & 0 deletions core/src/main/scala-2.13+/cats/compat/FoldableCompat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package cats
package compat

private[cats] trait FoldableCompat[F[_]] { this: Foldable[F] =>

def iterable[A](fa: F[A]): LazyList[A] =
foldRight[A, LazyList[A]](fa, Eval.now(LazyList.empty)) { (a, eb) =>
eb.map(LazyList.cons(a, _))
}.value
}

private[cats] object FoldableCompat {

trait ToFoldableCompatOps {
implicit final def foldableCompatSyntax[F[_], A](fa: F[A]): FoldableCompatAllOps[F, A] =
new FoldableCompatAllOps(fa)
}

final class FoldableCompatAllOps[F[_], A](private val fa: F[A]) extends AnyVal {
def iterable(implicit F: Foldable[F]): LazyList[A] = F.iterable(fa)
}
}
2 changes: 2 additions & 0 deletions core/src/main/scala-2.13+/cats/instances/lazyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ trait LazyListInstances extends cats.kernel.instances.LazyListInstances {

override def toList[A](fa: LazyList[A]): List[A] = fa.toList

override def iterable[A](fa: LazyList[A]): LazyList[A] = fa

override def reduceLeftOption[A](fa: LazyList[A])(f: (A, A) => A): Option[A] =
fa.reduceLeftOption(f)

Expand Down
2 changes: 2 additions & 0 deletions core/src/main/scala-2.13+/cats/instances/stream.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances {

override def toList[A](fa: Stream[A]): List[A] = fa.toList

override def iterable[A](fa: Stream[A]): LazyList[A] = LazyList.from(fa)

override def reduceLeftOption[A](fa: Stream[A])(f: (A, A) => A): Option[A] =
fa.reduceLeftOption(f)

Expand Down
9 changes: 5 additions & 4 deletions core/src/main/scala/cats/Foldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import Foldable.sentinel
*
* See: [[http://www.cs.nott.ac.uk/~pszgmh/fold.pdf A tutorial on the universality and expressiveness of fold]]
*/
@typeclass trait Foldable[F[_]] extends UnorderedFoldable[F] { self =>
@typeclass trait Foldable[F[_]] extends cats.compat.FoldableCompat[F] with UnorderedFoldable[F] { self =>

/**
* Left associative fold on 'F' using the function 'f'.
Expand Down Expand Up @@ -266,15 +266,16 @@ import Foldable.sentinel
* Fold implemented using the given Monoid[A] instance.
*/
def fold[A](fa: F[A])(implicit A: Monoid[A]): A =
foldLeft(fa, A.empty) { (acc, a) =>
A.combine(acc, a)
}
A.combineAll(iterable(fa))

/**
* Alias for [[fold]].
*/
def combineAll[A: Monoid](fa: F[A]): A = fold(fa)

def combineAllOption[A](fa: F[A])(implicit ev: Semigroup[A]): Option[A] =
if (isEmpty(fa)) None else ev.combineAllOption(iterable(fa))
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have iterable now, should we use it in fold for the default implementation? I think we should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it could be implemented in terms of the Monoid's combineAll like

A.combineAll(iterable(fa))

this would allow to optimize the fold providing a specialization for Monoid[A].
I'll add it in.


/**
* Fold implemented by mapping `A` values into `B` and then
* combining them using the given `Monoid[B]` instance.
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/scala/cats/syntax/foldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package cats
package syntax

trait FoldableSyntax extends Foldable.ToFoldableOps with UnorderedFoldable.ToUnorderedFoldableOps {
trait FoldableSyntax
extends Foldable.ToFoldableOps
with cats.compat.FoldableCompat.ToFoldableCompatOps
with UnorderedFoldable.ToUnorderedFoldableOps {

implicit final def catsSyntaxNestedFoldable[F[_]: Foldable, G[_], A](fga: F[G[A]]): NestedFoldableOps[F, G, A] =
new NestedFoldableOps[F, G, A](fga)

Expand Down
14 changes: 14 additions & 0 deletions tests/src/test/scala/cats/tests/FoldableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,20 @@ abstract class FoldableSuite[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
}
}

test(s"Foldable[$name].combineAllOption") {
forAll { (fa: F[Int]) =>
fa.combineAllOption should ===(fa.toList.combineAllOption)
fa.combineAllOption should ===(iterator(fa).toList.combineAllOption)
}
}

test(s"Foldable[$name].iterable") {
forAll { (fa: F[Int]) =>
fa.iterable.toList should ===(fa.toList)
fa.iterable.toList should ===(iterator(fa).toList)
}
}

test(s"Foldable[$name].intercalate") {
forAll { (fa: F[String], a: String) =>
fa.intercalate(a) should ===(fa.toList.mkString(a))
Expand Down