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

rustdoc: extend UdpSocket API doc (#657) #44378

Merged
merged 1 commit into from
Sep 15, 2017
Merged

rustdoc: extend UdpSocket API doc (#657) #44378

merged 1 commit into from
Sep 15, 2017

Conversation

frehberg
Copy link
Contributor

@frehberg frehberg commented Sep 6, 2017

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@frehberg
Copy link
Contributor Author

frehberg commented Sep 6, 2017

@dtolnay please review

Copy link
Contributor

@mattico mattico left a comment

Choose a reason for hiding this comment

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

Some ideas about the wording

@@ -48,12 +48,12 @@ use time::Duration;
/// {
/// let mut socket = UdpSocket::bind("127.0.0.1:34254")?;
///
/// // read from the socket
/// // read from the socket a single message, `buf` must be of sufficient size to hold the message
Copy link
Contributor

Choose a reason for hiding this comment

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

// Read a single message from the socket.
// If `buf` is too small to hold the message, it will be cut off.

/// let mut buf = [0; 10];
/// let (amt, src) = socket.recv_from(&mut buf)?;
///
/// // send a reply to the socket we received data from
/// let buf = &mut buf[..amt];
/// let buf = &mut buf[..amt]; // slice as bounded view onto `buf`
Copy link
Contributor

Choose a reason for hiding this comment

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

// create a slice into the buffer that's no longer than the received data

@aidanhs
Copy link
Member

aidanhs commented Sep 7, 2017

Thanks for the PR @frehberg! We'll check in now and again to make sure @aturon or another reviewer gets to this soon.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2017
@dtolnay dtolnay assigned dtolnay and unassigned aturon Sep 7, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! This is a nice improvement.

Both of my comments apply elsewhere in this PR as well.

@@ -48,12 +48,14 @@ use time::Duration;
/// {
/// let mut socket = UdpSocket::bind("127.0.0.1:34254")?;
///
/// // read from the socket
/// // Read a single message from the socket. If `buf` is too small to hold
/// // the message, it will be cut off.
Copy link
Member

Choose a reason for hiding this comment

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

// Read a single message from the socket. If `buf` is too small to hold
// the message, it will be cut off.
let mut buf = [0; 10];
let (amt, src) = socket.recv_from(&mut buf)?;

// send a reply to the socket we received data from
let buf = &mut buf[..amt]; // create a slice into the buffer that's
                           // no longer than the received data

This feels jarring to me in that it uses a mix of sentence-case and lowercase, punctuation and no punctuation, and full line and end of line comments. Could you tidy this up to make it feel more consistent? Typically I prefer sentence-case, with punctuation, and comments without any code on the same line. But check the rest of this module because if everything else is consistently in a different style, let's follow that.

/// of bytes read and the address from whence the data came.
/// The function must be called with valid byte array `buf` of sufficient size to
/// hold the message bytes. If a message is too long to fit in the supplied buffer,
/// excess bytes may be discarded.
Copy link
Member

Choose a reason for hiding this comment

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

Quoting from the documentation documentation:

The first line of a documentation comment should be a short summary of its functionality. One sentence. Just the basics. High level.

(I think by "line" they mean "paragraph" -- everything before the first blank line. The first paragraph is special in rustdoc. For example it is visible in the module-level documentation before you click through on the function.)

Everything you wrote is important to document but not all of it is high level basics. Could you split this into two paragraphs? The first one should give a general understanding of what the function does, and the second fills in the details.

@frehberg
Copy link
Contributor Author

frehberg commented Sep 8, 2017

Text has been modified, hope it fits now

@steveklabnik
Copy link
Member

[00:02:54] tidy error: /checkout/src/libstd/net/udp.rs:107: trailing whitespace

[00:02:54] tidy error: /checkout/src/libstd/net/udp.rs:139: trailing whitespace

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Almost looks good to me but please rephrase all three other comments to be consistent with the one I indicated. There are a few inconsistencies.

Read a single message from the socket
Receives a single datagram message from socket
Receives a single datagram message on the socket
Receives single datagram on the socket

... on success returning number of bytes
On success returning number of bytes
On success, returns the number of bytes

/// Receives data on the socket from the remote address to which it is
/// connected.
/// Receives a single datagram message on the socket from the remote address to
/// which it is connected. On success, returns the number of bytes read.
Copy link
Member

Choose a reason for hiding this comment

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

This one is great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one more iteration ;)

@steveklabnik
Copy link
Member

[00:03:28] tidy error: /checkout/src/libstd/net/udp.rs:107: trailing whitespace

[00:03:28] tidy error: /checkout/src/libstd/net/udp.rs:139: trailing whitespace

@dtolnay
Copy link
Member

dtolnay commented Sep 10, 2017

I don't think #44378 (review) has been addressed yet.

@frehberg
Copy link
Contributor Author

finally. Thanks for your patience

@steveklabnik
Copy link
Member

No problem! Thanks for keeping with it!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 11, 2017

📌 Commit 04b8747 has been approved by steveklabnik

@dtolnay
Copy link
Member

dtolnay commented Sep 11, 2017

On success returning the number of bytes read.

I don't think this sentence is grammatically correct -- just like "Going fishing" is not a complete sentence. I would prefer to word this (and the other ones similarly) like the one I indicated in #44378 (review):

On success, returns the number of bytes read.

@bors r-

@frehberg
Copy link
Contributor Author

finally, finally

@frehberg
Copy link
Contributor Author

Ready to merge?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

@bors r+ rollup

@dtolnay
Copy link
Member

dtolnay commented Sep 14, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit 85a9d97 has been approved by dtolnay

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 15, 2017
bors added a commit that referenced this pull request Sep 15, 2017
@bors bors merged commit 85a9d97 into rust-lang:master Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants