-
-
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
Lazy foldM for "Iterables" #1414
Conversation
Resolves typelevel#1030, though it may be desirable change the default implementation to be lazy in the future. This can be done with `FreeT` but as of now I'm not sure how to do it without.
Current coverage is 91.67% (diff: 100%)@@ master #1414 diff @@
==========================================
Files 240 240
Lines 3608 3617 +9
Methods 3540 3555 +15
Messages 0 0
Branches 67 61 -6
==========================================
+ Hits 3308 3316 +8
- Misses 300 301 +1
Partials 0 0
|
Some structures, such as |
@@ -372,4 +378,12 @@ object Foldable { | |||
Eval.defer(if (it.hasNext) f(it.next, loop()) else lb) | |||
loop() | |||
} | |||
|
|||
def iteratorFoldM[M[_], A, B](it: Iterator[A], z: B)(f: (B, A) => M[B])(implicit M: Monad[M]): M[B] = { |
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.
How about we also mention here in the doc the incentive of this special implementation for Iteratable
s?
The code seems good to me. 👍 Thanks! |
def iteratorFoldM[M[_], A, B](it: Iterator[A], z: B)(f: (B, A) => M[B])(implicit M: Monad[M]): M[B] = { | ||
val go: ((B, Iterator[A])) => M[Either[(B, Iterator[A]), B]] = { case (b, it) => | ||
if (it.hasNext) M.map(f(b, it.next))(b1 => Left((b1, it))) | ||
else M.pure(Right(b)) |
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.
Since Iterator
is mutable, I would keep it private to the method (otherwise we lose referential transparency). I would keep the signature as you had before, and only use Iterator
inside.
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.
Sorry if this was not clear from my previous comment.
Not sure what I did wrong that in this conversation it shows my newest comment as outdated. I will repeat here: Since Sorry if this was not clear from my previous comment. |
@TomasMikula in the past we've exposed other methods using |
Well, in that case, ship it! |
Another, perhaps premature, optimization would be to avoid allocating the tuple in each step by accessing the iterator from the outer scope instead of passing it along. |
LGTM 👍 |
381aa3d
to
6f3da09
Compare
👍 thanks very much! |
Yeah, I'm concerned everything except List will be slower here.
|
If I am understanding correctly, now that
FlatMap
hastailRecM
we can revisit #1030.I adapted an old commit by @ceedubs for a PR to add
MonadRec
by @TomasMikula.