Skip to content
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

Merged

Conversation

thomaseizinger
Copy link
Contributor

Description

This reduces the noise within core/src/either.rs.

Links to any relevant issues

Will reduce diff in #2648.

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

This reduces the noise within core/src/either.rs and will reduce
the diff in libp2p#2648.
Copy link
Member

@mxinden mxinden left a 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?

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor Author

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?

Copy link
Member

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)? 😇

@thomaseizinger thomaseizinger changed the title core/muxing: Introduce StreamMuxerEvent::map_stream core/muxing: Introduce StreamMuxerEvent::map_inbound_stream Jun 14, 2022
core/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger merged commit 3c120ef into libp2p:master Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants