-
Notifications
You must be signed in to change notification settings - Fork 1k
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
core/muxing: Introduce StreamMuxerEvent::map_inbound_stream
#2691
core/muxing: Introduce StreamMuxerEvent::map_inbound_stream
#2691
Conversation
This reduces the noise within core/src/either.rs and will reduce the diff in libp2p#2648.
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.
Looks good to me. Very much in favor of these small helper methods. Thanks for pushing for these.
Mind adding a changelog entry and bumping the libp2p-core
patch version?
core/src/muxing.rs
Outdated
@@ -241,6 +241,16 @@ impl<T> StreamMuxerEvent<T> { | |||
None | |||
} | |||
} | |||
|
|||
/// Map the stream within [`StreamMuxerEvent::InboundSubstream`] to a new type. | |||
pub fn map_stream<O>(self, map: impl FnOnce(T) -> O) -> StreamMuxerEvent<O> { |
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.
pub fn map_stream<O>(self, map: impl FnOnce(T) -> O) -> StreamMuxerEvent<O> { | |
pub fn map_inbound_stream<O>(self, map: impl FnOnce(T) -> O) -> StreamMuxerEvent<O> { |
Just to avoid the confusion of folks expecting the StreamMuxerEvent
to somehow contain both inbound and outbound streams. What do you think?
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 will contain both streams once #2648 is merged. If we do this rename now, I will have to rename it again :)
What do you prefer?
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.
Though extra work, I would prefer the rename. That way I can merge to master
already, keeping master
clean. The two pull requests might not be released in the same release. Mind doing the change (twice)? 😇
StreamMuxerEvent::map_stream
StreamMuxerEvent::map_inbound_stream
Description
This reduces the noise within core/src/either.rs.
Links to any relevant issues
Will reduce diff in #2648.
Open Questions
Change checklist