-
Notifications
You must be signed in to change notification settings - Fork 961
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
transports/tcp: Unify symbol naming #2961
Conversation
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.
👍
Co-authored-by: Elena Frank <[email protected]>
…/rust-libp2p into 2173-rename-tcp-transport-symbols
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.
LGMT 👍
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.
Sorry, two things just came to my mind when I though about following the same naming for the QuicTransport;
- How will
tokio::Transport
andasync_io::Transport
be show on docs.rs iflib::Transport
is annotated with#[doc(hdiden)]
? I believe if we mark this as hidden user will see the type aliases for each provider, but not be able to the see the public interface ofTransport
, thus also not what method it has and what traits are implemented on it. Is this correct? if so, I'd be strongly in favor of not hidingTransport
. - Why are we in general not exporting
GenTransport
andProvider
to the user, if they (for whatever reason) want to implement a custom provider? We already have this abstraction, why not expose it?
I am linking to the
We are exporting Regarding |
I ended up removing the |
@thomaseizinger is this ready for another review? If so @elenaf9 can you take another look and check whether your concerns are addressed? |
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.
Sorry for the delay. LGTM!
Yep this is ready to review. Will fix the merge conflicts. |
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.
As mentioned before, big fan of the first-deprecate-then-remove path now. Thanks for making libp2p-tcp
consistent with the recent renames.
I am in favor of merging here. Though I think we should tackle libp2p/test-plans#59 first.
Description
This follows the effort described in #2217. Instead of exporting one
TcpTransport
and aTokioTcpTransport
, we use the already existing modules to define aTransport
type alias.The recommended way of referencing these new transports from the root
libp2p
crate is:use libp2p::tcp;
tcp::async_io::Transport
ortcp::tokio::Transport
.Links to any relevant issues
Open Questions
Change checklist
I have added tests that prove my fix is effective or that my feature works