mergeAll doesn't wait every stream produces an item #137
Conversation
pavel-khritonenko
changed the title
pavel-khritonenko
force-pushed
the
master
branch
from
8a69b63
to
a08e33e
Compare
pavel-khritonenko
changed the title
|
Unfortunately, I don't know how to write more tests :-/ @polytypic, could you please review it when you have spare time? |
| @@ -1161,7 +1161,7 @@ module Stream = | |||
| val ambAll: Stream<#Stream<'x>> -> Stream<'x> | |||
|
|
|||
| /// Joins all the streams together with `merge`. | |||
| val mergeAll: Stream<#Stream<'x>> -> Stream<'x> | |||
| val mergeAll: Stream<Stream<'x>> -> Stream<'x> | |||
There was a problem hiding this comment.
Hmm... This signature change should be unnecessary.
| | Cons (x, xs) -> mergeAll' (merge combined x) xs :> _ | ||
| |> memo | ||
|
|
||
| let mergeAll producer = mergeAll' nil producer |
There was a problem hiding this comment.
After a quick look this seems like it should work correctly. It starts combined from nil, the identity element of merge, and only stops after both the combined and the producer stop.
| <|> producer ^=> function | ||
| | Nil -> combined >>= function | ||
| | Nil -> nilj | ||
| | Cons (v, vs) -> consj v (mergeAll' vs nil) |
There was a problem hiding this comment.
In the case that the producer ends, you should just be able to return combined.
| | Nil -> producer >>= function | ||
| | Nil -> nilj | ||
| | Cons (x, xs) -> mergeAll' x xs :> _ | ||
| | Cons (v, vs) -> consj v (mergeAll' vs producer) |
There was a problem hiding this comment.
The case when combined ends is like initial call to mergeAll (except for memoization).
| | Nil -> nilj | ||
| | Cons (v, vs) -> consj v (mergeAll' vs nil) | ||
| | Cons (x, xs) -> mergeAll' (merge combined x) xs :> _ | ||
| |> memo |
There was a problem hiding this comment.
Here could also just use <|>*.
|
|
||
| values ^-> Choice1Of2 | ||
| <|> timeOutMillis 100 ^-> Choice2Of2 | ||
| |> run = Choice1Of2 (xs @ [ () ]) |
There was a problem hiding this comment.
It is good to have tests!
It might be possible to make tests more deterministic, though I've also used non-deterministic global timer based tests myself.
|
Thanks for the PR! The tests are welcome and I checked blame and recalled this fairly recent change I made: That commit broke The reasons in favour of that change are not very strong. As said in the commit message, and as far as I can see, it only fixes a space leak in a degenerate use case of Perhaps it would be best to keep the new tests from this PR and revert the commit that broke |
It's hard to say from my point of view, I just want working functionality. Having ad-hoc implementation for |
fixes #135
I haven't managed to fix
joinWithmethod because of some type inference conflicts, so I decided to write an ad-hoc implementation ofmergeAllfirst to review.