-
-
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
Add existsM
and forallM
to Foldable
#1784
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1784 +/- ##
==========================================
+ Coverage 94.87% 94.88% +<.01%
==========================================
Files 241 241
Lines 4139 4147 +8
Branches 103 110 +7
==========================================
+ Hits 3927 3935 +8
Misses 212 212
Continue to review full report at Codecov.
|
I think if you use def existsM[G[_], A](fa: F[A])(p: A => G[Boolean])(implicit G: Monad[G]): G[Boolean] = {
val src = Foldable.Source.fromFoldable(fa)(self)
G.tailRecM(src) { srcc => srcc.uncons match {
case Some((a, src)) => G.map(p(a))(bb => if (bb) Right(true) else Left(src.value))
case None => G.pure(Right(false))
}}
} |
* res0: Option[Boolean] = Some(true) | ||
* | ||
* scala> F.existsM(List(1,2,3,4))(n => if (n <= 2) Option(true) else Option(false)) | ||
* res1: Option[Boolean] = Some(true) |
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.
might be better to have a negative test case here, e.g.
* scala> F.existsM(List(1,2,3,4))(n => if (n > 4) Option(true) else Option(false))
* res1: Option[Boolean] = Some(false)
@kailuowang Yes, but do we want it to short-circuit and skip effects? I could imagine an argument either way. For my use case, short-circuiting would probably be fine, but I didn’t want to presume that for everyone. IIRC scalaz's intentionally doesn’t shortcircuit, but I don’t know the rationale. |
I think there is definitely a use case for short-circuiting implementation. If we have to pick from the two, I would argue we should pick the short-circuit one, because the other one isn't that cumbersome to write on use-site if needed. How about we ask for more opinions on gitter or something? |
100% agreement. I meant to go back and say that short-circuiting would probably be more efficient in my use case; since the effects typically involve extra throwaway computation. |
@kailuowang I was mistaken when I said scalaz doesn't short-circuit; it does:
I'll redo the PR when I get a chance. |
err. not sure if you need to close this PR. If you replace with the implementation I gave it shortcircuit and pass the tests. |
Ok, I wasn't sure if I should create a new one, or rebase or what. |
* }}} | ||
*/ | ||
def forallM[G[_], A](fa: F[A])(p: A => G[Boolean])(implicit G: Monad[G]): G[Boolean] = { | ||
val src = Foldable.Source.fromFoldable(fa)(self) |
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.
This isn't being used right.
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.
I meant to delete line 393, which isn't used, but do you want to elaborate?
819f9d4
to
9800e4a
Compare
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.
👍
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.
awesome! thanks!
I will add an example to the |
I was missing these utility functions. I've been out of the loop for a while so let me know if I didn't contrib right, and I'll be happy to update.