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

Improve AddrParseError documentation. #48853

Merged

Conversation

Songbird0
Copy link
Contributor

I've added potential cause to AddrParseError raising.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2018
@Songbird0
Copy link
Contributor Author

r? @GuillaumeGomez

@joshtriplett
Copy link
Member

I like the premise of this change, adding information about what can cause an error. However, there's a certain tone this documentation takes that I'd rather we avoid. In particular, "rustc isn't angry anymore" seems problematic.

Also, "because of wrong resource typing" doesn't quite make sense. "because the provided string does not parse as the given type, often because it includes information only handled by a different address type", perhaps?

@Songbird0
Copy link
Contributor Author

Songbird0 commented Mar 8, 2018

@joshtriplett

In particular, "rustc isn't angry anymore" seems problematic.

Yes, I'll replace that with a more neutral sentence.

edit:

"because the provided string does not parse as the given type, often because it includes information only handled by a different address type"

I'm fine with that too. That's more exhaustive.

@Centril Centril added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Mar 8, 2018
@Songbird0
Copy link
Contributor Author

The changes were applied. Thanks!

///
/// ```
/// use std::net::SocketAddr;
/// // That's it. `rustc` isn't angry anymore.
/// // No problem, `panic!` message has disappear.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "has disappeared" rather than "has disappear". Also, need a "the" before the panic!.

Looks good otherwise, thanks for the update!

/// # Potential causes:
///
/// `AddrParseError` may be thrown because the provided string does not parse as the given type,
/// often because it includes informations only handled by a different address type.
Copy link
Member

Choose a reason for hiding this comment

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

"information" is invariable. "pieces of information"

///
/// ```
/// use std::net::SocketAddr;
/// // No problem, the `panic!` message has disappeared.
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty line.

Copy link
Contributor Author

@Songbird0 Songbird0 Mar 9, 2018

Choose a reason for hiding this comment

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

Done. I hope that's what you wanted. I didn't know if it was for line 389 or 390.

Copy link
Member

Choose a reason for hiding this comment

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

No, it's the correct place. :)

@Songbird0
Copy link
Contributor Author

If that's ok for you, that should be ok for me too. :)

@GuillaumeGomez
Copy link
Member

It's time to squash.

@Songbird0 Songbird0 force-pushed the addrparseerror_documentation_improvement branch from a1f0565 to 0931715 Compare March 13, 2018 13:15
@Songbird0
Copy link
Contributor Author

Done!

@@ -372,6 +372,25 @@ impl FromStr for SocketAddr {
/// [`IpAddr`], [`Ipv4Addr`], [`Ipv6Addr`], [`SocketAddr`], [`SocketAddrV4`], and
/// [`SocketAddrV6`].
///
/// # Potential causes:
///
/// `AddrParseError` may be thrown because the provided string does not parse as the given type,
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, forgot to ask you to turn this into an url (like done in a few places here too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current page we are reading is the AddrParseError page. It seemed useless to me, but, ok, I'll apply the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah then my bad. Just ignore this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too fast, I had applied the changes. :)

@@ -372,6 +372,25 @@ impl FromStr for SocketAddr {
/// [`IpAddr`], [`Ipv4Addr`], [`Ipv6Addr`], [`SocketAddr`], [`SocketAddrV4`], and
/// [`SocketAddrV6`].
///
/// # Potential causes:
Copy link
Member

Choose a reason for hiding this comment

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

Please remove ":".

///
/// ```should_panic
/// use std::net::IpAddr;
/// let _foo: IpAddr = "127.0.0.1:8080".parse().unwrap(); // panic!
Copy link
Member

Choose a reason for hiding this comment

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

We should never use unwrap in examples (or in code more generally). At worst, please use expect.

/// use std::net::SocketAddr;
///
/// // No problem, the `panic!` message has disappeared.
/// let _foo: SocketAddr = "127.0.0.1:8080".parse().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same.

/// ```
///
/// [`FromStr`]: ../../std/str/trait.FromStr.html
/// [`AddrParseError`]: ../../std/net/struct.AddrParseError.html
Copy link
Member

Choose a reason for hiding this comment

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

Just like you said, it's useless to do this since we're already on the correct page (sorry again about this...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

Yes, it was removed. See 11be095203a084733966e9d52b1c0489bbd22056.

Add a potential cause to `AddrParseError` raising.
@Songbird0 Songbird0 force-pushed the addrparseerror_documentation_improvement branch from 11be095 to 5e991e1 Compare March 15, 2018 18:27
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Mar 15, 2018

📌 Commit 5e991e1 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Mar 15, 2018
…ion_improvement, r=GuillaumeGomez

Improve `AddrParseError` documentation.

I've added potential cause to `AddrParseError` raising.
bors added a commit that referenced this pull request Mar 16, 2018
Rollup of 17 pull requests

- Successful merges: #48706, #48875, #48892, #48922, #48957, #48959, #48961, #48965, #49007, #49024, #49042, #49050, #48853, #48990, #49037, #49049, #48972
- Failed merges:
@bors bors merged commit 5e991e1 into rust-lang:master Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants