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

jxs · 2022-11-22T22:44:11Z

Description

Notes

Continues addressing #2217.
Per commit review is suggested :)

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

and RequestResponseHandlerEvent to HandlerEvent.

protocols/request-response/src/lib.rs

thomaseizinger

Nice work! Thanks :)

thomaseizinger · 2022-11-22T23:02:35Z

protocols/request-response/src/handler.rs

 #[doc(hidden)]
-pub enum RequestResponseHandlerEvent<TCodec>
+pub enum HandlerEvent<TCodec>


Would just Event be pushing it too far? :D

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

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.

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?

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.

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

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;

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?

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

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.

protocols/request-response/src/handler.rs

mxinden

Other than the comments by Thomas above, this looks good to me.

Thanks @jxs.

mxinden · 2022-11-23T15:28:57Z

protocols/request-response/src/handler.rs

 #[doc(hidden)]
-pub enum RequestResponseHandlerEvent<TCodec>
+pub enum HandlerEvent<TCodec>


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.

protocols/request-response/CHANGELOG.md

…quest-response

…elease

mxinden

This is missing a version bump in the root Cargo.toml, right?

protocols/request-response/CHANGELOG.md

mxinden · 2022-12-02T16:41:21Z

protocols/request-response/src/codec.rs

 /// protocol family and how they are encoded / decoded on an I/O stream.
+#[deprecated(
+    since = "0.23.0",


Suggested change

since = "0.23.0",

since = "0.24.0",

mxinden · 2022-12-02T16:41:34Z

protocols/request-response/src/handler.rs

-#[doc(hidden)]
-pub struct RequestResponseHandler<TCodec>
+#[deprecated(
+    since = "0.23.0",


Suggested change

since = "0.23.0",

since = "0.24.0",

Thanks Max, addressed.

mxinden · 2022-12-02T16:41:46Z

protocols/request-response/src/handler.rs

-#[doc(hidden)]
-pub enum RequestResponseHandlerEvent<TCodec>
+#[deprecated(
+    since = "0.23.0",


Suggested change

since = "0.23.0",

since = "0.24.0",

Thanks Max, addressed.

thomaseizinger

Thanks for doing this!

A few minor comments :)

protocols/autonat/src/behaviour.rs

protocols/request-response/src/codec.rs

protocols/request-response/src/handler.rs

protocols/request-response/src/lib.rs

…quest-response

protocols/request-response/src/codec.rs

mxinden

Changes look good to me. No objections to merging other than the missing version change and changelog entry.

@thomaseizinger any remaining comments?

mxinden · 2022-12-06T18:28:21Z

Cargo.toml

@@ -106,7 +106,7 @@ libp2p-plaintext = { version = "0.38.0", path = "transports/plaintext", optional
 libp2p-pnet = { version = "0.22.2", path = "transports/pnet", optional = true }
 libp2p-relay = { version = "0.14.0", path = "protocols/relay", optional = true }
 libp2p-rendezvous = { version = "0.11.0", path = "protocols/rendezvous", optional = true }
-libp2p-request-response = { version = "0.23.0", path = "protocols/request-response", optional = true }
+libp2p-request-response = { version = "0.24.0", path = "protocols/request-response", optional = true }


Given that this is a breaking change for libp2p, this pull request needs to bump the libp2p version to v0.51.0. Can you please also add a changelog entry to the root CHANGELOG.md file. See previous entries for reference.

thomaseizinger · 2022-12-07T04:54:47Z

@thomaseizinger any remaining comments?

Nope, LGTM!

thomaseizinger · 2022-12-07T04:59:30Z

@thomaseizinger any remaining comments?

Nope, LGTM!

Actually, I do want to finish one discussion: #3159 (comment)

mergify · 2022-12-09T14:46:41Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…quest-response

thomaseizinger

Lovely, thanks so much!

I am thrilled that this is backwards-compatible now :)

mergify · 2022-12-12T11:32:40Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…quest-response

jxs · 2022-12-12T18:28:41Z

fixed the merge conflicts, and updated the deprecation message to 0.24

mergify · 2022-12-12T21:00:05Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…quest-response

…tion (libp2p#3159)

jxs added 7 commits November 22, 2022 21:51

request-response: rename RequestResponseMessage to Message

5ec3449

request-response: rename RequestResponseHandler to Handler,

744c2fb

and RequestResponseHandlerEvent to HandlerEvent.

request-response: rename RequestResponseCodec to Codec

d0c69a8

request-response: rename RequestResponseEvent to Event

bf3c663

request-response: rename RequestResponseConfig to Config

abd326c

request-response: rename RequestResponse to Behaviour

8dcf4b5

request-response: remove [doc(hidden)] from Handler

f5fcc29

jxs commented Nov 22, 2022

View reviewed changes

protocols/request-response/src/lib.rs Show resolved Hide resolved

thomaseizinger reviewed Nov 22, 2022

View reviewed changes

thomaseizinger mentioned this pull request Nov 23, 2022

All crates should have a consistent API & naming #2217

Open

mxinden reviewed Nov 23, 2022

View reviewed changes

review: rename HandlerEvent to handler::Event

6d998d4

jxs force-pushed the rename-request-response branch 2 times, most recently from 149a2c5 to eddda25 Compare November 24, 2022 11:43

jxs added 3 commits November 24, 2022 12:19

review: re-create deprcated RequestResponseCodec.

3693a79

review: improve request-response Handler doc wording.

d2880cc

Merge github.com:libp2p/rust-libp2p into rename-request-response

0e42929

jxs force-pushed the rename-request-response branch from eddda25 to d2880cc Compare November 24, 2022 12:20

mxinden reviewed Dec 1, 2022

View reviewed changes

protocols/request-response/CHANGELOG.md Outdated Show resolved Hide resolved

jxs added 2 commits December 1, 2022 20:09

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

00465e4

…quest-response

review: Update CHANGELOG.md to reflect libp2p-request-response 0.23 r…

57bb47f

…elease

mxinden reviewed Dec 2, 2022

View reviewed changes

thomaseizinger reviewed Dec 4, 2022

View reviewed changes

jxs added 2 commits December 4, 2022 13:35

review: update version on missing places

11ca1df

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

637a32d

…quest-response

jxs force-pushed the rename-request-response branch from f7e1c43 to 637a32d Compare December 4, 2022 13:36

jxs added 2 commits December 4, 2022 15:17

review: apply Thomas wording suggestions

dabd8bc

review: blanket Codec impl for types that impl RequestResponseCodec.

82c0e85

thomaseizinger reviewed Dec 4, 2022

View reviewed changes

protocols/request-response/src/codec.rs Outdated Show resolved Hide resolved

review: rename pending HandlerEvent references.

9813119

jxs added 2 commits December 5, 2022 01:07

review: make blanket Codec implementation #[async_trait].

0b5eb4c

review: improve wording on doc

9be0603

mxinden reviewed Dec 6, 2022

View reviewed changes

thomaseizinger approved these changes Dec 7, 2022

View reviewed changes

thomaseizinger changed the title ~~protocols/request-response: Revise symbol naming~~ refactor(request-response): revise public API to follow naming convention Dec 8, 2022

jxs added 3 commits December 9, 2022 15:07

review: update release to patch instead of minor

84dd335

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

183f88d

…quest-response

review: rename dangling 0.24 references.

1f3fbab

thomaseizinger approved these changes Dec 12, 2022

View reviewed changes

mxinden approved these changes Dec 12, 2022

View reviewed changes

jxs added 2 commits December 12, 2022 18:20

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

533ae05

…quest-response

review: update deprecated messages back to 0.24 version

223ec5f

jxs force-pushed the rename-request-response branch from 3dc01fe to 223ec5f Compare December 12, 2022 18:27

jxs added 2 commits December 12, 2022 21:17

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

12094b4

…quest-response

merge: replace removed webrtc smoke test functions

7cd250c

thomaseizinger approved these changes Dec 13, 2022

View reviewed changes

thomaseizinger added the send-it label Dec 13, 2022

jxs and others added 4 commits December 13, 2022 10:17

review: clippy

c030232

Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…

fb07b18

…quest-response

review: fix docs

a613a94

Merge branch 'master' into rename-request-response

6db0668

mergify bot merged commit f828db6 into libp2p:master Dec 13, 2022

jxs mentioned this pull request Dec 16, 2022

refactor(relay): revise public API to follow naming convention #3238

Merged

4 tasks

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024

refactor(request-response): revise public API to follow naming conven…

369337e

…tion (libp2p#3159)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jxs commented Nov 22, 2022

thomaseizinger left a comment

thomaseizinger Nov 22, 2022

thomaseizinger Nov 22, 2022 •

edited

Loading

mxinden Nov 23, 2022

jxs Nov 24, 2022

thomaseizinger Dec 4, 2022

jxs Dec 4, 2022

thomaseizinger Dec 7, 2022

thomaseizinger Dec 7, 2022

jxs Dec 7, 2022

thomaseizinger Dec 8, 2022

mxinden left a comment

mxinden Nov 23, 2022

mxinden left a comment

mxinden Dec 2, 2022

mxinden Dec 2, 2022

jxs Dec 4, 2022

mxinden Dec 2, 2022

jxs Dec 4, 2022

thomaseizinger left a comment

mxinden left a comment

mxinden Dec 6, 2022

thomaseizinger commented Dec 7, 2022

thomaseizinger commented Dec 7, 2022

mergify bot commented Dec 9, 2022

thomaseizinger left a comment

mergify bot commented Dec 12, 2022

jxs commented Dec 12, 2022

mergify bot commented Dec 12, 2022

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

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

Conversation

jxs commented Nov 22, 2022

Description

Notes

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Dec 7, 2022

thomaseizinger commented Dec 7, 2022

mergify bot commented Dec 9, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 12, 2022

jxs commented Dec 12, 2022

mergify bot commented Dec 12, 2022

thomaseizinger Nov 22, 2022 •

edited

Loading