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.

Sign up for GitHub

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

Jump to bottom

refactor(request-response): revise public API to follow naming convention #3159

Merged
merged 31 commits into from
Dec 13, 2022
Merged

refactor(request-response): revise public API to follow naming convention #3159

Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
5ec3449
request-response: rename RequestResponseMessage to Message
jxs Nov 22, 2022
744c2fb
request-response: rename RequestResponseHandler to Handler,
jxs Nov 22, 2022
d0c69a8
request-response: rename RequestResponseCodec to Codec
jxs Nov 22, 2022
bf3c663
request-response: rename RequestResponseEvent to Event
jxs Nov 22, 2022
abd326c
request-response: rename RequestResponseConfig to Config
jxs Nov 22, 2022
8dcf4b5
request-response: rename RequestResponse to Behaviour
jxs Nov 22, 2022
f5fcc29
request-response: remove [doc(hidden)] from Handler
jxs Nov 22, 2022
6d998d4
review: rename HandlerEvent to handler::Event
jxs Nov 24, 2022
3693a79
review: re-create deprcated RequestResponseCodec.
jxs Nov 24, 2022
d2880cc
review: improve request-response Handler doc wording.
jxs Nov 24, 2022
0e42929
Merge github.com:libp2p/rust-libp2p into rename-request-response
jxs Nov 24, 2022
00465e4
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 1, 2022
57bb47f
review: Update CHANGELOG.md to reflect libp2p-request-response 0.23 r…
jxs Dec 1, 2022
11ca1df
review: update version on missing places
jxs Dec 4, 2022
637a32d
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 4, 2022
dabd8bc
review: apply Thomas wording suggestions
jxs Dec 4, 2022
82c0e85
review: blanket Codec impl for types that impl RequestResponseCodec.
jxs Dec 4, 2022
9813119
review: rename pending HandlerEvent references.
jxs Dec 5, 2022
0b5eb4c
review: make blanket Codec implementation #[async_trait].
jxs Dec 5, 2022
9be0603
review: improve wording on doc
jxs Dec 5, 2022
84dd335
review: update release to patch instead of minor
jxs Dec 9, 2022
183f88d
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 9, 2022
1f3fbab
review: rename dangling 0.24 references.
jxs Dec 9, 2022
533ae05
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 12, 2022
223ec5f
review: update deprecated messages back to 0.24 version
jxs Dec 12, 2022
12094b4
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 12, 2022
7cd250c
merge: replace removed webrtc smoke test functions
jxs Dec 12, 2022
c030232
review: clippy
jxs Dec 13, 2022
fb07b18
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 13, 2022
a613a94
review: fix docs
jxs Dec 13, 2022
6db0668
Merge branch 'master' into rename-request-response
mergify[bot] Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions protocols/request-response/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ use std::{
)]
pub type RequestResponseHandler<TCodec> = Handler<TCodec>;

/// A connection handler of a `Behaviour` protocol.
#[doc(hidden)]
/// A connection handler of a [`Behaviour`](super::Behaviour) protocol.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
pub struct Handler<TCodec>
where
TCodec: Codec,
Expand Down Expand Up @@ -202,7 +201,6 @@ where
pub type RequestResponseHandlerEvent<TCodec> = HandlerEvent<TCodec>;

/// The events emitted by the [`Handler`].
#[doc(hidden)]
pub enum HandlerEvent<TCodec>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would just Event be pushing it too far? :D

Copy link
Contributor

@thomaseizinger thomaseizinger Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In case it isn't I think the handler module should be private so this Event can't clash with the other one.

Copy link
Member

Choose a reason for hiding this comment

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

Intuitively libp2p::request_response::handler::Event reads better than libp2p::request_response::handler::HandlerEvent and would be inline with the convention this pull request is enforcing.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah makes sense to me, thanks for the suggestion,
So, there is request_response::Event and request_response::handler::Event now. Can you expand on Event's clashing Thomas?
There will still be request_response::RequestResponseHandlerEvent which is deprecated and suggests request_response::handler::Event, if we make handler private how can we export both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah if we make handler private, we can no longer export it. What I meant with clashing is that from the outside, there is really only one event that is interesting for users and it is the NetworkBehaviour's OutEvent.

Given that we are already making this a breaking change, we might as well make handler private. In my opinion, no one should be depending on just the ConnectionHandler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation Thomas. Handler and Event can't be private because NetworkBehaviour::ConnectionBehaviour associated type needs to be public, and by consequence ConnectionHandler's also

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation Thomas. Handler and Event can't be private because NetworkBehaviour::ConnectionBehaviour associated type needs to be public, and by consequence ConnectionHandler's also

Hmm that is news to me! We have a lot of protocols where the ConnectionHandler implementation is completely private to the crate. For example ping:

rust-libp2p/protocols/ping/src/lib.rs

Line 45 in 5755942

mod handler;

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested it locally and things compile if I make the handler module private. However, I'd suggest that we don't do this for now. I think together with #3159 (comment), we can make this completely non-breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for digging this deeper Thomas! Did you understand what is it that with request-response that was making the compiler complain?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue is fairly subtle:

If you have a public type that implements a trait, the compiler enforces that the associated types also need to be public. It does however not check whether the type is within a public or private module in regards to the crate.

In our usecase, this means we can have a pub struct Handler that implements ConnectionHandler but define it within a crate-private module handler. This may or may not be a bug in Rust but I think one can argue that it makes sense:

The API contract promised to the user is that Behaviour implements NetworkBehaviour. NetworkBehaviour enforces that NetworkBehaviour::ConnectionHandler implements IntoConnectionHandler. The user can refer to this type as <Behaviour::ConnectionHandler as IntoConnectionHandler> and call all public APIs on that. They don't need to know the concrete type that implements the functionality. This way, only the minimal promised API is actually exposed.

where
TCodec: Codec,
Expand Down