-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Rework tokio_tcp and tokio_udp re-exports in tokio::net #548
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.
Thanks for doing this 👍 I like the type alias trick you used to deprecate the export. I used it myself elsewhere just now.
I left some notes inline.
//! | ||
//! This module contains the TCP/UDP networking types, similar to the standard | ||
//! This module contains the TCP/UDP/Unix networking types, similar to the standard |
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.
I agree that the current documentation should be moved into the appropriate sub module, but that leaves tokio::net
documentation a bit light, especially given that it is a primary module.
Perhaps, you could add an "orgnaization" section, similar to https://doc.rust-lang.org/std/net/index.html.
Also, this can mention the unix module and explain that it is only available on unix platforms.
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.
Looks like this change was made in 7ea54a0.
src/net.rs
Outdated
pub use self::tcp::{TcpListener, TcpStream}; | ||
|
||
#[deprecated(note = "use `tokio::net::tcp::ConnectFuture` instead")] | ||
/// Convenience re-export of [`tcp::ConnectFuture`](tcp/struct.ConnectFuture.html). Will be removed in a future release. |
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.
I would just remove docs on deprecated items and also include a #[doc(hidden)]
annotation.
src/net.rs
Outdated
|
||
#[cfg(unix)] | ||
pub mod unix { |
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.
I'm a bit torn about this, but I would suggest we stick to the current idiom of having the cfg on the entire module vs. the types inside.
I think we can achieve the same level of documentation by having a # Unix
section in tokio::net
explaining how it works.
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.
Again, looks like this change was made in 7ea54a0.
Any updates? |
Actually yes, just waited for some time to submit the changes. I think I addressed all your comments. Also, I went to great lengths to add platform specific documentation to get a result similar to |
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.
It looks like 7ea54a0 made all the changes @carllerche requested.
//! | ||
//! This module contains the TCP/UDP networking types, similar to the standard | ||
//! This module contains the TCP/UDP/Unix networking types, similar to the standard |
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.
Looks like this change was made in 7ea54a0.
src/net.rs
Outdated
|
||
#[cfg(unix)] | ||
pub mod unix { |
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.
Again, looks like this change was made in 7ea54a0.
This is the continuation of #526. I applied the same pattern than before.
To signify deprecation I had to use type aliases since it not possible to deprecate a re-export for now (see rust-lang/rust#30827 for details).
I also split the documentation in the different modules. Documentation wording isn't really my forte so feel free to suggest enhancements on this side.
Last thing I made the
unix
module present but empty on non-unix systems. I'm not sure this is the best idea but it allows completeness on all systems.