-
-
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
added whileM, untilM, iterateWhile, etc to Monad #1571
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1571 +/- ##
==========================================
+ Coverage 92.34% 92.41% +0.07%
==========================================
Files 247 248 +1
Lines 3907 3944 +37
Branches 132 147 +15
==========================================
+ Hits 3608 3645 +37
Misses 299 299
Continue to review full report at Codecov.
|
core/src/main/scala/cats/Monad.scala
Outdated
* returns `true`. The condition is evaluated before the loop body. | ||
* Discards results. | ||
*/ | ||
def whileM_[A](p: F[Boolean], body: => F[A]): F[Unit] = { |
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.
why not use the same style as above(p: F[Boolean])(body: => F[A])
?
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.
meant to make them the same, but let it slipped.
core/src/main/scala/cats/Monad.scala
Outdated
* returns `true`. The condition is evalated before the loop body. | ||
* Collects the results into an arbitrary `MonadCombine` value, such as a `List`. | ||
*/ | ||
def whileM[G[_], A](p: F[Boolean])(body: => F[A])(implicit G: MonadCombine[G]): F[G[A]] = { |
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.
can we comment why one of these params is call by name, but not the other. It is not clear to me. Same comment below.
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 was under the impression that we don't use by-name params in cats.
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.
@johnynek I think the intention here is to avoid evaluating the body F[A]
if p
is never true. Thanks to you, I noticed that I missed the memoization of body. I just implemented using a Later
.
@edmundnoble internally we don't use by-name params, but we do have plenty of public APIs using it for the sake of ease to use when we don't really want to give user the control over evaluation.
In this particular case, having user passing in a by-name and then memoize it might be misleading. I am open to change to take in a Later[A]
instead. Or maybe it's okay to give user the control and just take in an Eval
. What do you guys think?
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.
@kailuowang Oh, I didn't know it was solely an optimization to avoid evaluating the body. I don't see any reason to let the user supply an Eval because it will be evaluated more than once; otherwise makes sense to me.
tailRecM[G[A], G[A]](G.empty)(xs => ifM(p)( | ||
ifTrue = { | ||
map(b.value) { bv => | ||
Left(G.combineK(xs, G.pure(bv))) |
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.
Notice here that to maintain the order of evaluation results I have to append the latest result to the end of the Alternative
, which means the performance will be horrible for List
.
I see 3 options here:
- in the API document, point this out and warn people against data structures with non-constant append performance.
- use prepend here and return the
G
with results in the reverse order, i.e. last result first. - maintain an internal
List
in the reverse order and construct theG
at last in the right order.
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 think it's fine to just document the behavior, rather than trying to be clever. This is a general issue with accumulating structures.
def whileM_[A](p: F[Boolean])(body: => F[A]): F[Unit] = { | ||
val continue: Either[Unit, Unit] = Left(()) | ||
val stop: F[Either[Unit, Unit]] = pure(Right(())) | ||
val b = Eval.later(body) |
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.
why the later vs always semantics? For most monads it shouldn't matter, but for something like Future
or Try
where there is possibly a side-effect going on, it would.
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.
Do you think these methods would be of practical use for Future
or Try
? For the conditional param p
, neither a Try[Boolean]
nor Future[Boolean]
will ever change the underneath value, and it's call by value.
It seems to me that the original incentive of introducing these methods are only for data types that track side effects such as IO
/Task
/StateT
. When it comes to other data types like Option
or List
, they don't make sense to me, at least with current type signatures.
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 agree with @kailuowang. This is for data types that have something to say about side-effects or state-passing. Future
and Try
do not.
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.
That's fine, I was mostly asking because of the Eval.later
. If we used Eval.always
then you could use Try
or Future
by sneaking some effect into the call-by-name param, but I'm not a fan of that.
If we want to stick with Eval.later
over lazy val, I'd like us to document the reason. The only reason I can see is that it allows the thunk in the call-by-name to be GC'ed after we run, which can be nice. We should comment why because someone will forget and revert since Later is only more work (it uses lazy val + some work).
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 agree we should document these not-so-obvious tech decisions somewhere (another example in my mind is typeclass instance priority). Maybe in the contributing?
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.
A few comments but this looks good to me.
tailRecM[G[A], G[A]](G.empty)(xs => ifM(p)( | ||
ifTrue = { | ||
map(b.value) { bv => | ||
Left(G.combineK(xs, G.pure(bv))) |
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 think it's fine to just document the behavior, rather than trying to be clever. This is a general issue with accumulating structures.
def whileM_[A](p: F[Boolean])(body: => F[A]): F[Unit] = { | ||
val continue: Either[Unit, Unit] = Left(()) | ||
val stop: F[Either[Unit, Unit]] = pure(Right(())) | ||
val b = Eval.later(body) |
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 agree with @kailuowang. This is for data types that have something to say about side-effects or state-passing. Future
and Try
do not.
def untilM[G[_], A](f: F[A])(cond: => F[Boolean])(implicit G: Alternative[G]): F[G[A]] = { | ||
val p = Eval.later(cond) | ||
flatMap(f)(x => map(whileM(map(p.value)(!_))(f))(xs => G.combineK(G.pure(x), xs))) | ||
} |
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.
So, is val p = Eval.later(cond)
what we're doing instead of lazy val p = cond
? Is there a discussion somewhere?
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.
Yes. I picked Eval.later(cond)
over lazy val
because of the optimization in Later
, and the fact that an equivalent Need
is used in scalaz. But I don't have proof that it is worth the extra allocation.
I am not aware of any discussion regarding choosing between these two in cats.
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.
Ok, I don't feel strongly about it. Was just curious.
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.
Later just uses lazy val under the hood: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Eval.scala#L186
The only reason I can think of to use Later
vs lazy val
is maybe the thunk can be GC'd in the Later case? cc @non
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's also a locking issue -- lazy vals lock their enclosing class, Later
only locks itself.
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.
Yeah see scalaz/scalaz#1144 for discussion (moving my comment up).
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.
okay. Makes sense to me. 👍
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.
looks good to me with comments added about Eval.later vs lazy val.
def whileM_[A](p: F[Boolean])(body: => F[A]): F[Unit] = { | ||
val continue: Either[Unit, Unit] = Left(()) | ||
val stop: F[Either[Unit, Unit]] = pure(Right(())) | ||
val b = Eval.later(body) |
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.
That's fine, I was mostly asking because of the Eval.later
. If we used Eval.always
then you could use Try
or Future
by sneaking some effect into the call-by-name param, but I'm not a fan of that.
If we want to stick with Eval.later
over lazy val, I'd like us to document the reason. The only reason I can see is that it allows the thunk in the call-by-name to be GC'ed after we run, which can be nice. We should comment why because someone will forget and revert since Later is only more work (it uses lazy val + some work).
Continuation on #1216. I didn't add the other methods yet because I want to make sure I get the tests right first. There are no tests present in either original PR or Scalaz .
cc @tpolecat @johnynek
Fixes #1569.