-
-
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
remove iteratorFoldM fixes #1716 #1740
Changes from 7 commits
33352a2
ba99572
9705b47
e2f94dc
e955595
a8b54a9
e2f53ae
312ee35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,8 +90,14 @@ trait ListInstances extends cats.kernel.instances.ListInstances { | |
|
||
override def filter[A](fa: List[A])(f: A => Boolean): List[A] = fa.filter(f) | ||
|
||
override def foldM[G[_], A, B](fa: List[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | ||
Foldable.iteratorFoldM(fa.toIterator, z)(f) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we maybe do this one? def step(in: (List[A], B])): G[Either[(List[A], B), A]] = in match {
case (Nil, b) => G.pure(Right(b))
case (a :: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
}
G.tailRecM((fa, z))(step) right? If we had this, we could cover the common case for |
||
override def foldM[G[_], A, B](fa: List[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = { | ||
def step(in: (List[A], B)): G[Either[(List[A], B), B]] = in match { | ||
case (Nil, b) => G.pure(Right(b)) | ||
case (a :: tail, b) => G.map(f(b, a)) { bnext => Left((tail, bnext)) } | ||
} | ||
|
||
G.tailRecM((fa, z))(step) | ||
} | ||
|
||
override def fold[A](fa: List[A])(implicit A: Monoid[A]): A = A.combineAll(fa) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package cats | |
package instances | ||
|
||
import cats.syntax.show._ | ||
|
||
import scala.annotation.tailrec | ||
|
||
trait StreamInstances extends cats.kernel.instances.StreamInstances { | ||
|
@@ -118,8 +119,17 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances { | |
|
||
override def collect[A, B](fa: Stream[A])(f: PartialFunction[A, B]): Stream[B] = fa.collect(f) | ||
|
||
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | ||
Foldable.iteratorFoldM(fa.toIterator, z)(f) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could copy the def step(in: (Stream[A], B])): G[Either[(Stream[A], B), A]] = in match {
case (Stream.empty, b) => G.pure(Right(b))
case (a #:: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
}
G.tailRecM((fa, z))(step) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patten match Stream into |
||
override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good, but now again the default implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops. I'll correct the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated. I also added the original redundant tests back to test the stream override implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we would profit if had some machinery like mentioned in #1684. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomasMikula I think we are still tested with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup. I thought about that too. |
||
def step(in: (Stream[A], B)): G[Either[(Stream[A], B), B]] = { | ||
val (s, b) = in | ||
if(s.isEmpty) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scalastyle seems to want a space between the |
||
G.pure(Right(b)) | ||
else | ||
G.map(f(b, s.head)) { bnext => Left((s.tail, bnext)) } | ||
} | ||
|
||
G.tailRecM((fa, z))(step) | ||
} | ||
|
||
override def fold[A](fa: Stream[A])(implicit A: Monoid[A]): A = A.combineAll(fa) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import cats.syntax.show._ | |
import scala.annotation.tailrec | ||
import scala.collection.+: | ||
import scala.collection.immutable.VectorBuilder | ||
import list._ | ||
|
||
trait VectorInstances extends cats.kernel.instances.VectorInstances { | ||
implicit val catsStdInstancesForVector: TraverseFilter[Vector] with MonadCombine[Vector] with CoflatMap[Vector] = | ||
|
@@ -91,7 +92,7 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |
override def collect[A, B](fa: Vector[A])(f: PartialFunction[A, B]): Vector[B] = fa.collect(f) | ||
|
||
override def foldM[G[_], A, B](fa: Vector[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | ||
Foldable.iteratorFoldM(fa.toIterator, z)(f) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Foldable[List].foldM(fa.toList, z)(f) | ||
|
||
override def fold[A](fa: Vector[A])(implicit A: Monoid[A]): A = A.combineAll(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.
NonEmptyList
sort of gets this implementation already throughNonEmptyReducible
, but I guess this saves us thesplit
.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, forgot about that. A single split call probably doesn't justify the code duplication. I am going to remove this.