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

Add protocols_handler::multi module. #1497

Merged
merged 12 commits into from
Mar 26, 2020
Merged

Conversation

twittner
Copy link
Contributor

An implementation of ProtocolsHandler that contains multiple other ProtocolsHandlers indexed by some key type.

Factored out from paritytech/substrate#5045.

An implementation of `ProtocolsHandler` that contains multiple other
`ProtocolsHandler`s indexed by some key type.
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.

For what my review is worth, this looks good to me.

Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

You also need to provide an implementation of IntoProtocolsHandler.

swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
@twittner
Copy link
Contributor Author

You also need to provide an implementation of IntoProtocolsHandler.

My understanding was that there is a blanket impl of IntoProtocolsHandler for types that implement ProtocolsHandler.

@tomaka
Copy link
Member

tomaka commented Mar 19, 2020

My understanding was that there is a blanket impl of IntoProtocolsHandler for types that implement ProtocolsHandler.

All ProtocolsHandlers implement IntoProtocolsHandler, but not the invert.
Right now you can't use a "multi handler" with an H that implements only IntoProtocolsHandler and not ProtocolsHandler.

To rephrase: the "protocols handler" thing is really two steps (the "into", then the actual handler), and for convenience we allow short-circuiting the first step. But in that PR you make it mandatory to short-circuit the first step, which is not great for something generic.

swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Show resolved Hide resolved
swarm/src/protocols_handler/multi.rs Outdated Show resolved Hide resolved
- No more `Debug` bound for the key type and more generic log messages.
- Additional comments.
- Imports instead of fully-qualified use.
- Renamed `DuplicateProtoname` to `DuplicateProtonameError`.
@tomaka tomaka merged commit 0d3e4f2 into libp2p:master Mar 26, 2020
folex added a commit to fluencelabs/rust-libp2p that referenced this pull request Mar 30, 2020
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.

4 participants