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

tcp: add usage examples to TcpListener and TcpStream #775

Merged

Conversation

mattgathu
Copy link
Contributor

@mattgathu mattgathu commented Nov 26, 2018

Motivation

Request by the Networking Working Group to help document tokio.
rustasync/team#54

Solution

Adding doc tests and usage examples to the public methods
of TcpListener and TcpStream. The documented methods are:

  • tokio-tcp/src/listener.rs#bind
  • tokio-tcp/src/listener.rs#accept
  • tokio-tcp/src/listener.rs#poll_accept
  • tokio-tcp/src/listener.rs#accept_std
  • tokio-tcp/src/listener.rs#poll_accept_std
  • tokio-tcp/src/listener.rs#from_std
  • tokio-tcp/src/listener.rs#local_addr
  • tokio-tcp/src/listener.rs#incoming
  • tokio-tcp/src/listener.rs#ttl
  • tokio-tcp/src/listener.rs#set_ttl
  • tokio-tcp/src/stream.rs#connect
  • tokio-tcp/src/stream.rs#from_std
  • tokio-tcp/src/stream.rs#poll_read_ready
  • tokio-tcp/src/stream.rs#poll_write_ready
  • tokio-tcp/src/stream.rs#local_addr
  • tokio-tcp/src/stream.rs#peer_addr
  • tokio-tcp/src/stream.rs#peek
  • tokio-tcp/src/stream.rs#poll_peek
  • tokio-tcp/src/stream.rs#shutdown
  • tokio-tcp/src/stream.rs#nodelay
  • tokio-tcp/src/stream.rs#set_nodelay
  • tokio-tcp/src/stream.rs#recv_buffer_size
  • tokio-tcp/src/stream.rs#set_recv_buffer_size
  • tokio-tcp/src/stream.rs#send_buffer_size
  • tokio-tcp/src/stream.rs#set_send_buffer_size
  • tokio-tcp/src/stream.rs#keepalive
  • tokio-tcp/src/stream.rs#set_keepalive
  • tokio-tcp/src/stream.rs#ttl
  • tokio-tcp/src/stream.rs#set_ttl
  • tokio-tcp/src/stream.rs#linger
  • tokio-tcp/src/stream.rs#set_linger
  • tokio-tcp/src/stream.rs#try_clone

Refs: rustasync/team#54

Adding doc tests and usage examples to the public methods
of `TcpListener` and `TcpStream`. The documented methods are:
- tokio-tcp/src/listener.rs#bind
- tokio-tcp/src/listener.rs#accept
- tokio-tcp/src/listener.rs#poll_accept
- tokio-tcp/src/listener.rs#accept_std
- tokio-tcp/src/listener.rs#poll_accept_std
- tokio-tcp/src/listener.rs#from_std
- tokio-tcp/src/listener.rs#local_addr
- tokio-tcp/src/listener.rs#incoming
- tokio-tcp/src/listener.rs#ttl
- tokio-tcp/src/listener.rs#set_ttl
- tokio-tcp/src/stream.rs#connect
- tokio-tcp/src/stream.rs#from_std
- tokio-tcp/src/stream.rs#poll_read_ready
- tokio-tcp/src/stream.rs#poll_write_ready
- tokio-tcp/src/stream.rs#local_addr
- tokio-tcp/src/stream.rs#peer_addr
- tokio-tcp/src/stream.rs#peek
- tokio-tcp/src/stream.rs#poll_peek
- tokio-tcp/src/stream.rs#shutdown
- tokio-tcp/src/stream.rs#nodelay
- tokio-tcp/src/stream.rs#set_nodelay
- tokio-tcp/src/stream.rs#recv_buffer_size
- tokio-tcp/src/stream.rs#set_recv_buffer_size
- tokio-tcp/src/stream.rs#send_buffer_size
- tokio-tcp/src/stream.rs#set_send_buffer_size
- tokio-tcp/src/stream.rs#keepalive
- tokio-tcp/src/stream.rs#set_keepalive
- tokio-tcp/src/stream.rs#ttl
- tokio-tcp/src/stream.rs#set_ttl
- tokio-tcp/src/stream.rs#linger
- tokio-tcp/src/stream.rs#set_linger
- tokio-tcp/src/stream.rs#try_clone

Refs: rustasync/team#54
@mattgathu
Copy link
Contributor Author

This is my first pass at documenting the Struct methods. Looking forward to your feedback.

@tobz
Copy link
Member

tobz commented Nov 27, 2018

@mattgathu One question, because my knowledge of doctests is minimal: it looks like some of the lines have leading # and some don't.. i.e. all of the extern crate lines seem to have it, but not all use lines do.

Why is that?

@mattgathu
Copy link
Contributor Author

@tobz the # is a way comment out those lines in the rendered documentation. Some lines have to be there for the doc tests to be run correctly but do not need to appear in the documentation

@tobz
Copy link
Member

tobz commented Nov 27, 2018

Ahhh, TIL!

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

I actually went back and did a better review... and it actually looks like some of the examples comment out all of the use statements. Is that... correct? Obviously it will still compile but I had taken the lack of the leading # on some of them to be a show of what needed to be used for types that were being shown in the code... and some of these examples theoretically won't show any use statements at all, despite utilizing some of these types.

/// # use futures::Async;
/// # use futures::Future;
/// # use tokio_tcp::TcpStream;
/// # use std::net::SocketAddr;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's some inconsistency here where we aren't showing any of the use statements at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I wasn't sure exactly what to show and what to hide. Do you have a preference or rule of thumb I could follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing to consider is that the TcpListener and TcpStream are mostly not used in isolation, which makes it a bit hard to provide meaningful examples.

For instance, the tinyhttp.rs in Examples has this snippet:

    let addr = env::args().nth(1).unwrap_or("127.0.0.1:8080".to_string());
    let addr = addr.parse::<SocketAddr>()?;

    let listener = TcpListener::bind(&addr)?;
    println!("Listening on: {}", addr);

    tokio::run({
        listener.incoming()
            .map_err(|e| println!("failed to accept socket; error = {:?}", e))
            .for_each(|socket| {
                process(socket);
                Ok(())
            })
    });

This actually shows how to use the TcpListener with the Tokio runtime and could be really useful to have in the documentation but this is hard - the doctests will fail since the runtime is not part of the tcp crate.

Copy link
Member

Choose a reason for hiding this comment

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

@mattgathu You can include the tokio runtime in the doc tests. Add a dev-dependency to tokio in the tokio-tcp crate 👍

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM Thanks so much for doing this ❤️

Re @tobz thoughts, up until now, we haven't really had a good rule of thumb about what to include. However, it probably is best to include more than less, including extern crate and use statements.

That said, we can open an issue to discuss the conventions and address it later across the all the examples in the repo.

@carllerche
Copy link
Member

@mattgathu I'm happy to merge this whenever. I commented above that you can depend on the Tokio runtime in the doc examples for each crate. If you want to do that, go for it.

Let me know when you want this merged 👍

@mattgathu
Copy link
Contributor Author

@carllerche awesome. let me update the docs and include tokio and then this can be merged. Thanks for the feedback.

* update the doc tests/usage examples to use tokio::net.
* made all `use` statements visible
* simplified some examples (use consistent wording)
* added `no_run` to tests that may cause block or cause port contention
@mattgathu
Copy link
Contributor Author

mattgathu commented Nov 27, 2018

@tobz @carllerche I have updated the docs:

  • used tokio::net in all examples instead of tokio_tcp (more consistent for users)
  • made all use statements visible.
  • used consistent naming

you might give it a quick glance before merging: 42052c2

@tobz
Copy link
Member

tobz commented Nov 28, 2018

Yep, looks good to me! :)

@tobz tobz merged commit 1cd0ebf into tokio-rs:master Nov 28, 2018
@tobz
Copy link
Member

tobz commented Nov 28, 2018

And, it goes without saying: thank you for your contribution!

@carllerche
Copy link
Member

❤️ ❤️ ❤️ 🌮

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.

None yet

3 participants