feat: add typed iterators for StreamMap and StreamSet#10
Conversation
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Looks great! I left a few comments with nits.
I don't think this is a breaking change because the docs for Any say:
Most types implement Any. However, any type which contains a non-'static reference does not. See the module-level documentation for more details.
We already require all streams and futures to be 'static so I think we are fine here. Having said that, I don't think it is a big deal either way and we can just also publish another minor release.
| } | ||
| assert_eq!(streams.iter_typed::<mpsc::Receiver<()>>().count(), N); | ||
| for (i, (id, _)) in streams.iter_typed::<mpsc::Receiver<()>>().enumerate() { | ||
| let expect_id = format!("ID{}", N - i - 1); // Reverse order. |
There was a problem hiding this comment.
Huh? Why are we handing them out in reverse order?
There was a problem hiding this comment.
Not sure why, but that's how the inner SelectAll::iter returns the the elements. Probably an implementation detail of the SelectAll data structure?
There was a problem hiding this comment.
Interesting. It is a Vec internally. Should we sort them by ID?
There was a problem hiding this comment.
I don't think we can sort them in the SelectAll, can we? Or where/ how should we sort them?
There was a problem hiding this comment.
We could sort them as part of the Iterator implementation. itertools has a combinator for that.
There was a problem hiding this comment.
This would be better left to the caller: callers who want a sorted iterator can use the combinator, and those who don't care don't have to pay the performance cost of sorting/comparisons (and extra temporary memory).
Documenting that the order is unspecified should be enough to inform people of the need for sorting.
There was a problem hiding this comment.
Documenting that the order is unspecified should be enough to inform people of the need for sorting.
Makes sense to me! Wanna send a PR?
|
Awesome! Thanks for pushing this forward. I'll merge this to |
@elenaf9 Let me know once you need a release here. |
This is on my list of things to do, but I don't know how many days or weeks it will be until it reaches the top of the list. So @elenaf9 feel free to go ahead (and close my |
3 participants
Based on discussion in #8 and draft #9.
Notes
I renamed the functions to
iter{_mut}_typedto avoid confusion with normaliter/iter_mutand emphasize that streams that can't be downcasted will be skipped. Wdyt @thomaseizingerAlso, a major version bump is needed because it breaks the API with the additional
Anybound, right?