Backport #2380 Add combineAllOption to Foldable
#3340
Conversation
Codecov Report
@@ Coverage Diff @@
## scala_2.11 #3340 +/- ##
==============================================
+ Coverage 93.41% 93.42% +<.01%
==============================================
Files 389 389
Lines 7294 7295 +1
Branches 187 188 +1
==============================================
+ Hits 6814 6815 +1
Misses 480 480
Continue to review full report at Codecov.
|
|
There's no need for |
| * not to force a full materialization of the collection. | ||
| */ | ||
| def toIterable(implicit F: Foldable[F]): Iterable[A] = | ||
| F.foldRight[A, Stream[A]](fa, Eval.now(Stream.empty))((a, eb) => eb.map(Stream.cons(a, _))).value |
There was a problem hiding this comment.
Hello, sorry to bother you again on this PR.
I was looking at the toIterable implementation to understand why would we use Stream.
These are the possible arguments that I read from the original PR-
Still aiming at mitigating the possible cost of building a list, it could be delayed up to when the iterator is traversing it
and
@travisbrown .toList.iterator forces a full materialization, and we don't control the API of List. So, the uses of a lazy structure (like Iterator, Stream, LazyList) are very different since we can interact with other tools that will not force a full materialization of the data. This is useful in things like spark or scalding.
That said, I think in a library focused on FP, we should use an immutable lazy structure if possible, so I would lean towards Stream/LazyList. Users can then call .iterator on that, but we don't control that API. Alternatively, they can use foldLeft to tear down the Stream without materializing the whole thing in memory at once.
this is when I added a few printLn in between, confirming my suspicion that the Stream is created from the end to the first element, this means that the entire Stream is already materialised yet not visible.
when replacing Stream with List, no difference.
and given this evidence we can replace this entire implementation with-
F.foldLeft(fa, List.empty[A])((acc, a) => a :: acc).reverse
Stream is materialised yet not visible then re-materialised (going through the whole synchronise thing).
Do you see value here?
There was a problem hiding this comment.
@travisbrown if you get a chance to go through this one, that's the last of it
addresses #3143