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.

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 iterateWhileM and iterateUntilM #1809

Merged
merged 4 commits into from
Aug 30, 2017

Conversation

drbild
Copy link
Contributor

@drbild drbild commented Aug 9, 2017

  • Adds iterateWhileM and iterateUntilM to complement the existing iterateWhile and iterateUntil methods.
  • Reimplements iterateWhile and iterateUntil in terms of the new iterateWhileM and iterateUntilM.

@kailuowang
Copy link
Contributor

looks great! Thanks! 👍 pending build pass.

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #1809 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1809      +/-   ##
==========================================
+ Coverage   94.96%   94.97%   +<.01%     
==========================================
  Files         241      241              
  Lines        4193     4195       +2     
  Branches      117      109       -8     
==========================================
+ Hits         3982     3984       +2     
  Misses        211      211
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/monad.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Monad.scala 96% <100%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f12f35c...60d8e5a. Read the comment docs.

@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Aug 9, 2017
@kailuowang kailuowang requested a review from tpolecat August 9, 2017 20:54
@peterneyens
Copy link
Collaborator

peterneyens commented Aug 9, 2017

Just wondering, these iterateUntil / iterateWhile methods only make sense for something like Task or IO, right ?
Are there cases where the f: A => F[A] would actually return something different every time, so you couldn't use f(a).iterateUntil(pred) instead of a.iterateUntilM(f)(pred) ?


Edit: I missed that in the M variants, the f is reused. Makes more sense now 😄. Thanks for this PR.
I would like to see some less trivial scaladoc examples as the ones using Id.

@drbild
Copy link
Contributor Author

drbild commented Aug 9, 2017

@peterneyens It's a common idiom in Haskell where A represents some state and A => F[A] is a side-effecting update function and you want to iterate until some terminal state is reached.

Simple games are a frequent example. A is the current state of the world. A => IO[A] is the step function that reads user input, updates the state accordingly, and refreshes the UI. A => Boolean checks for a termination condition like the player dying.

So the main loop becomes something like

def initialWorld: World = ??? // initial world state

def stepWorld(old): IO[World] = getUserInput >>= updateWorld(old) >>! refreshUI

def keepRunning(current): Boolean = isPlayerAlive(current)

def loop: IO[World] = initialWorld.iterateWhileM(stepWorld)(keepRunning)

This example could all be rewritten using StateT[IO, World, ?] so that

def loop = stepWorld.iterateWhile(keepRunning).run(initialWorld)

would work. But pulling in StateT often seems like overkill.

@drbild
Copy link
Contributor Author

drbild commented Aug 9, 2017

I think of iterateWhile as a specialized version of iterateWhileM. As an analogy:

iterateWhile : iterateWhileM :: followedBy : flatMap

@kailuowang
Copy link
Contributor

I agree that some less trivial tests/examples would be nice.

Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I agree it would be nice to see a test with something less trivial, like Eval maybe.

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

this is great, in my opinion, but I'd also like to see a more meaty monad in the tests. I'd like to see State or Reader for instance.

@@ -74,4 +74,28 @@ class MonadTest extends CatsSuite {
result should ===(50000)
}

test("iterateWhileM") {
forAll(Gen.posNum[Int]) { (max: Int) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

might this not run a super long time? any reason why max shouldn't be something from like: Gen.choose(0, 500000)?

* Apply a monadic function iteratively until its result fails
* to satisfy the given predicate and return that result.
*/
def iterateWhileM[A](init: A)(f: A => F[A])(p: A => Boolean): F[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the outer { } here.

* Apply a monadic function iteratively until its result satisfies
* the given predicate and return that result.
*/
def iterateUntilM[A](init: A)(f: A => F[A])(p: A => Boolean): F[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the outer { }

@drbild drbild force-pushed the add-iterateuntilm-etc branch 2 times, most recently from 5742379 to 3ae823b Compare August 14, 2017 17:00
if (p(a))
map(f(a))(_.asLeft[A])
else
pure(a.asRight[A])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we replace the asLeft and asRight with just Left(..) and Right(..) (in whileM as well) ?

@drbild drbild force-pushed the add-iterateuntilm-etc branch 5 times, most recently from bcfd5e6 to 075ae1f Compare August 20, 2017 22:01
@kailuowang
Copy link
Contributor

LGTM, @peterneyens @johnynek looks like that the comments were addressed. any other feedback?

@drbild
Copy link
Contributor Author

drbild commented Aug 28, 2017

I've left the Id-based example in the scaladoc, because nothing with a non-trivial monad but simple enough for a scaladoc has come to mind. I could drop the scaladoc entirely for now. Some simple iterative algorithm - like Euclid's GCD - but with a possible failure condition (so in the Option or Either monad) would be good. I just haven't been able come up with one.

I'm also working on some additions to the Monad doc, using some real examples - finding all solutions of a subset-sum variant and all solutions of the n-queens problem, both in the List monad. Those can go in a different PR though.

@kailuowang
Copy link
Contributor

I love the doc ideas, they sound awesome. I'd say we can have them in a separate PR so that we can have the binary breaking change merged in asap.

@drbild drbild force-pushed the add-iterateuntilm-etc branch from 075ae1f to 60d8e5a Compare August 30, 2017 00:58
@non
Copy link
Contributor

non commented Aug 30, 2017

Looks good to me. 👍

@kailuowang
Copy link
Contributor

since all feedback seems addressed and I am going to merge with 3 sign-offs.

@kailuowang kailuowang merged commit 406d984 into typelevel:master Aug 30, 2017
@drbild drbild deleted the add-iterateuntilm-etc branch August 30, 2017 16:53
LukaJCB pushed a commit to LukaJCB/cats that referenced this pull request Sep 1, 2017
* Limit runtime of monad loop tests

* Do not use Either syntax in monad loops

* Add iterateWhileM and iterateUntilM

* Implement iterateWhile in terms of iterateWhileM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants