Add filterFold and collectFold to Foldable #2452
Conversation
| def filterFold[A, M: Monoid](fa: F[A])(f: A ⇒ Option[M]): M = { | ||
| val m = Monoid[M] | ||
| foldLeft(fa, m.empty)((acc, a) ⇒ f(a) match { | ||
| case Some(x) ⇒ m.combine(x, acc) |
There was a problem hiding this comment.
I think the order on combine is wrong. I think this won’t match collectFold for a non commutative Monoid.
There was a problem hiding this comment.
Yes, that's right, thanks, I will fix it soon :)
| test(s"Foldable[$name] partial summation") { | ||
| forAll { (fa: F[Int], f: Int ⇒ Boolean) ⇒ | ||
| val m: Monoid[Int] = Monoid[Int] | ||
|
|
There was a problem hiding this comment.
Let’s test with a non commutative Monoid like String.
There was a problem hiding this comment.
Yes, l will do it, thanks.
|
The CI complains an binary compatibility issue: [error] * method filterFold(java.lang.Object,scala.Function1,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable.filterFold")
[error] * method collectFold(java.lang.Object,scala.PartialFunction,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable.collectFold")
[error] * method filterFold(scala.Function1,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable#Ops is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable#Ops.filterFold")
[error] * method collectFold(scala.PartialFunction,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable#Ops is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable#Ops.collectFold")Should I implement these two methods as syntax extensions to avoid this problem? |
Codecov Report
@@ Coverage Diff @@
## master #2452 +/- ##
=========================================
+ Coverage 95.29% 95.39% +0.1%
=========================================
Files 351 357 +6
Lines 6371 6519 +148
Branches 279 275 -4
=========================================
+ Hits 6071 6219 +148
Misses 300 300
Continue to review full report at Codecov.
|
| * res0: Int = 6 | ||
| *}}} | ||
| */ | ||
| def filterFold[M: Monoid](f: A ⇒ Option[M])(implicit F: Foldable[F]): M = { |
There was a problem hiding this comment.
def filterFold[M: Monoid](f: A ⇒ Option[M])(implicit F: Foldable[F]): M =
collectFold(Function.unlift(f))There was a problem hiding this comment.
or, the other way around:
def collectFold[M: Monoid](f: PartialFunction[A, M])(implicit F: Foldable[F]): M =
filterFold(f.lift)There was a problem hiding this comment.
We could indeed define these in terms of each other. I'm not sure if they'd be more performant than just using foldLeft, but if we do this, then it should be filterFold in terms of collectFold, since A => Option[M] has to allocate an extra Option :)
Up to you, @satansk :)
There was a problem hiding this comment.
Hi @PeterPerhac @LukaJCB , I think define them in terms of each other is good, but people are more familiar with foldLeft, so I personally prefer to define them by foldLeft :)
But if there are big performance differences, we can change to the better one.
| *}}} | ||
| */ | ||
| def collectFold[M: Monoid](f: PartialFunction[A, M])(implicit F: Foldable[F]): M = { | ||
| val m = Monoid[M] |
There was a problem hiding this comment.
totally nitpick - there are two styles of implicit type class instance summoning in this one method (and the filterFold method below). I think it would help readability a little bit if we just use (implicit F: Foldable[F], M: Monoid[M])
There was a problem hiding this comment.
This is a nice advice, it will be more readable, and I'll fix it :)
Thanks!
| * res0: Int = 6 | ||
| *}}} | ||
| */ | ||
| def filterFold[M](f: A ⇒ Option[M])(implicit F: Foldable[F], M: Monoid[M]): M = |
There was a problem hiding this comment.
The word filter, to me, kind of implies using a boolean value.
We already have a collectFirstSome https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Foldable.scala#L233
how about naming this one collectSomeFold?
There was a problem hiding this comment.
A reasonable advice, I'll fix it.
There was a problem hiding this comment.
We do have mapFilter which does use Option though.
Maybe this should be mapFilterFold? I don't know
There was a problem hiding this comment.
Right, I am fine with mapFilterFold too. Sorry about the bikeshedding @satansk , your call.
|
👍 |
|
@satansk +100 ：） |
|
@hepin1989 :) |
fix #2322