-
Notifications
You must be signed in to change notification settings - Fork 61
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
mergeAll doesn't wait every stream produces an item #137
base: master
Are you sure you want to change the base?
Conversation
8a69b63
to
a08e33e
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here could also just use <|>*
.
|
||
values ^-> Choice1Of2 | ||
<|> timeOutMillis 100 ^-> Choice2Of2 | ||
|> run = Choice1Of2 (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.
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
joinWith
method because of some type inference conflicts, so I decided to write an ad-hoc implementation ofmergeAll
first to review.