-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add Stream::flatten and Stream::flat_map #367
Conversation
The review is ready. Can you see it if you like? @stjepang |
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
Co-Authored-By: Taiki Endo <[email protected]>
@taiki-e Thanks for review |
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.
This PR is looking really good! -- left a comment on code structure, but feel this is very good already!
src/stream/stream/flatten.rs
Outdated
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
self.project().inner.poll_next(cx) | ||
} | ||
} |
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.
Instead of exporting flatten::{Flatten, FlatMap}
I think it'd be preferable to export them from separate files.
The FlattenCompat
is a nice idea, but I'm generally wary of introducing abstractions here, preferring to duplicate some of the logic instead. While this is more lines of code, it's also somewhat simpler. Would you be okay with splitting the two impls into two files, and removing the shared FlattenCompat
struct?
Thanks!
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.
Yh!
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.
yay, thanks!
Thanks! |
Implement Stream::flatten and Stream::flat_map
ref: #129