Improve `AddrParseError` documentation.#48853

Songbird0 · 2018-03-08T22:30:41Z

I've added potential cause to AddrParseError raising.

rust-highfive · 2018-03-08T22:30:52Z

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.

Songbird0 · 2018-03-08T22:30:56Z

r? @GuillaumeGomez

joshtriplett · 2018-03-08T22:49:42Z

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 · 2018-03-08T22:54:58Z

@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.

Songbird0 · 2018-03-08T23:16:59Z

The changes were applied. Thanks!

joshtriplett · 2018-03-08T23:56:02Z

src/libstd/net/parser.rs

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

Looks good otherwise, thanks for the update!

GuillaumeGomez · 2018-03-09T09:31:03Z

src/libstd/net/parser.rs

"information" is invariable. "pieces of information"

GuillaumeGomez · 2018-03-09T09:32:56Z

src/libstd/net/parser.rs

Please add an empty line.

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

No, it's the correct place. :)

Songbird0 · 2018-03-10T17:51:58Z

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

GuillaumeGomez · 2018-03-13T09:01:41Z

It's time to squash.

Songbird0 · 2018-03-13T13:18:36Z

Done!

GuillaumeGomez · 2018-03-14T08:53:30Z

src/libstd/net/parser.rs

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

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

Ah then my bad. Just ignore this comment.

Too fast, I had applied the changes. :)

GuillaumeGomez · 2018-03-14T08:53:37Z

src/libstd/net/parser.rs

Please remove ":".

GuillaumeGomez · 2018-03-14T08:54:07Z

src/libstd/net/parser.rs

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

GuillaumeGomez · 2018-03-14T08:54:16Z

src/libstd/net/parser.rs

GuillaumeGomez · 2018-03-15T10:33:30Z

src/libstd/net/parser.rs

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

Hi,

Yes, it was removed. See 11be095203a084733966e9d52b1c0489bbd22056.

Add a potential cause to `AddrParseError` raising.

GuillaumeGomez · 2018-03-15T21:21:15Z

Thanks!

@bors: r+ rollup

bors · 2018-03-15T21:21:16Z

📌 Commit 5e991e1 has been approved by GuillaumeGomez

…ion_improvement, r=GuillaumeGomez Improve `AddrParseError` documentation. I've added potential cause to `AddrParseError` raising.

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:

rust-highfive assigned joshtriplett Mar 8, 2018

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

rust-highfive assigned GuillaumeGomez and unassigned joshtriplett Mar 8, 2018

Centril added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Mar 8, 2018

joshtriplett reviewed Mar 8, 2018

View reviewed changes

GuillaumeGomez reviewed Mar 9, 2018

View reviewed changes

src/libstd/net/parser.rs Outdated

Copy link

Member

GuillaumeGomez Mar 9, 2018

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"

GuillaumeGomez reviewed Mar 9, 2018

View reviewed changes

Songbird0 force-pushed the addrparseerror_documentation_improvement branch from a1f0565 to 0931715 Compare March 13, 2018 13:15

GuillaumeGomez reviewed Mar 14, 2018

View reviewed changes

src/libstd/net/parser.rs Outdated

Copy link

Member

GuillaumeGomez Mar 14, 2018

Choose a reason for hiding this comment

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

Please remove ":".

GuillaumeGomez reviewed Mar 14, 2018

View reviewed changes

src/libstd/net/parser.rs Outdated

Copy link

Member

GuillaumeGomez Mar 14, 2018

Choose a reason for hiding this comment

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

Same.

GuillaumeGomez reviewed Mar 15, 2018

View reviewed changes

Improve AddrParseError documentation.

5e991e1

Add a potential cause to `AddrParseError` raising.

Songbird0 force-pushed the addrparseerror_documentation_improvement branch from 11be095 to 5e991e1 Compare March 15, 2018 18:27

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 mentioned this pull request Mar 15, 2018

Rollup of 17 pull requests #49051

Merged

bors merged commit 5e991e1 into rust-lang:master Mar 16, 2018

Uh oh!

Conversation

Songbird0 commented Mar 8, 2018

Uh oh!

rust-highfive commented Mar 8, 2018

Uh oh!

Songbird0 commented Mar 8, 2018

Uh oh!

joshtriplett commented Mar 8, 2018

Uh oh!

Songbird0 commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Songbird0 commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Songbird0 Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Songbird0 commented Mar 10, 2018

Uh oh!

GuillaumeGomez commented Mar 13, 2018

Uh oh!

Songbird0 commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Mar 15, 2018

Uh oh!

bors commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Songbird0 commented Mar 8, 2018 •

edited

Loading

Songbird0 Mar 9, 2018 •

edited

Loading