Update docs on Error struct. #29355 #42837

rthomas · 2017-06-22T20:12:34Z

This adds a pretty contrived example of the usage of fmt::Error. I am
very open to suggestions for a better one.

I have also highlighted the fmt::Error vs std::error::Error.

rust-highfive · 2017-06-22T20:12:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (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.

steveklabnik

some nits. I agree that the example is not spectacular, but I have no better suggestions right now, and so it's fine.

steveklabnik · 2017-06-27T21:37:23Z

src/libcore/fmt/mod.rs

@@ -81,6 +81,21 @@ pub type Result = result::Result<(), Error>;
 /// This type does not support transmission of an error other than that an error
 /// occurred. Any extra information must be arranged to be transmitted through
 /// some other means.
+///
+/// The key thing to remember here is that the type `fmt::Error` should not be


I would say "An important thing to remember is" instead, that is, this is very important, but not the thing, IMO

steveklabnik · 2017-06-27T21:37:47Z

src/libcore/fmt/mod.rs

+/// # Example
+///
+/// ```rust
+/// use std::fmt;


use std::fmt{self, write};

steveklabnik · 2017-06-27T21:37:57Z

src/libcore/fmt/mod.rs

+/// use std::fmt;
+///
+/// let mut output = String::new();
+/// match fmt::write(&mut output, format_args!("Hello {}!", "world")) {


match write

steveklabnik · 2017-06-27T21:38:31Z

src/libcore/fmt/mod.rs

+/// match fmt::write(&mut output, format_args!("Hello {}!", "world")) {
+///     Err(fmt::Error) => panic!("An error occurred"),
+///     _ => (),
+/// }


instead of match, using unwrap_or_else is more idiomatic here

I agree it would be more idiomatic, but that would not demonstrate the usage of fmt::Error - so I'm not sure if it would be worth including the example in that case.

QuietMisdreavus · 2017-06-27T21:42:37Z

src/libcore/fmt/mod.rs

@@ -81,6 +81,21 @@ pub type Result = result::Result<(), Error>;
 /// This type does not support transmission of an error other than that an error
 /// occurred. Any extra information must be arranged to be transmitted through
 /// some other means.
+///
+/// The key thing to remember here is that the type `fmt::Error` should not be
+/// confused with `std::error::Error`, which you may also have in scope.


If we're comparing to other Errors within the standard library, it might also be good to include std::io::Error, which I would consider more well-known than the Error trait.

rthomas · 2017-07-03T13:16:32Z

I've pushed up a commit with the review comments addressed in it.

The only potentially open issue is the example.

rthomas · 2017-07-05T08:18:13Z

Friendly ping.

steveklabnik · 2017-07-05T14:32:44Z

Sorry about this! I took Monday of work to get a four-day weekend 😄

I will review this later today.

steveklabnik

Hi! Sorry again. Two extremely tiny nits, and then let's . Ping me once they're changed and I will merge post-haste!

steveklabnik · 2017-07-05T18:06:58Z

src/libcore/fmt/mod.rs

@@ -1,3 +1,4 @@
+


can you remove this please?

steveklabnik · 2017-07-05T18:07:18Z

src/libcore/fmt/mod.rs

+/// confused with `std::io::Error` or `std::error::Error`, which you may also
+/// have in scope.
+///
+/// # Example


This should be Examples even though there's only one, for consistency.

rthomas · 2017-07-05T20:01:31Z

Thanks @steveklabnik I've fixed up the issues and pushed the commit up - would you like me to squash them down or are they fine as is?

rthomas · 2017-07-11T07:59:57Z

@steveklabnik friendly ping.

steveklabnik · 2017-07-11T23:18:14Z

@rthomas sorry, somehow I missed this!

Yeah, if you could squash, that'd be great. Happy to merge after 😄

@steveklabnik

This adds a pretty contrived example of the usage of fmt::Error. I am very open to suggestions for a better one. I have also highlighted the fmt::Error vs std::error::Error. r? @steveklabnik

rthomas · 2017-07-13T16:25:51Z

Thanks @steveklabnik, I have squashed the commits.

carols10cents · 2017-07-17T12:34:44Z

@steveklabnik looks like this is ready for your r!

steveklabnik · 2017-07-17T16:54:39Z

@bors: r+ rollup

thanks so much, and sorry again about the delays

bors · 2017-07-17T16:54:40Z

📌 Commit aca6cd0 has been approved by steveklabnik

@steveklabnik

Update docs on Error struct. rust-lang#29355 This adds a pretty contrived example of the usage of fmt::Error. I am very open to suggestions for a better one. I have also highlighted the fmt::Error vs std::error::Error. r? @steveklabnik

Rollup of 11 pull requests - Successful merges: #42837, #43282, #43287, #43290, #43292, #43294, #43304, #43310, #43312, #43314, #43315 - Failed merges:

rust-highfive assigned steveklabnik Jun 22, 2017

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2017

arielb1 assigned QuietMisdreavus and unassigned steveklabnik Jun 27, 2017

steveklabnik reviewed Jun 27, 2017

View reviewed changes

QuietMisdreavus reviewed Jun 27, 2017

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2017

steveklabnik suggested changes Jul 5, 2017

View reviewed changes

arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2017

steveklabnik approved these changes Jul 11, 2017

View reviewed changes

Update docs on Error struct. rust-lang#29355

aca6cd0

This adds a pretty contrived example of the usage of fmt::Error. I am very open to suggestions for a better one. I have also highlighted the fmt::Error vs std::error::Error. r? @steveklabnik

rthomas force-pushed the 29355-error branch from 3f6fded to aca6cd0 Compare July 13, 2017 16:25

Mark-Simulacrum mentioned this pull request Jul 18, 2017

Rollup of 11 pull requests #43316

Merged

bors added a commit that referenced this pull request Jul 18, 2017

Auto merge of #43316 - Mark-Simulacrum:rollup, r=Mark-Simulacrum

af049cd

Rollup of 11 pull requests - Successful merges: #42837, #43282, #43287, #43290, #43292, #43294, #43304, #43310, #43312, #43314, #43315 - Failed merges:

bors merged commit aca6cd0 into rust-lang:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docs on Error struct. #29355 #42837

Update docs on Error struct. #29355 #42837

rthomas commented Jun 22, 2017

rust-highfive commented Jun 22, 2017

steveklabnik left a comment

steveklabnik Jun 27, 2017

rthomas Jul 2, 2017

steveklabnik Jun 27, 2017

rthomas Jul 2, 2017

steveklabnik Jun 27, 2017

rthomas Jul 2, 2017

steveklabnik Jun 27, 2017

rthomas Jul 2, 2017

QuietMisdreavus Jun 27, 2017

rthomas Jul 2, 2017

rthomas commented Jul 3, 2017

rthomas commented Jul 5, 2017

steveklabnik commented Jul 5, 2017

steveklabnik left a comment

steveklabnik Jul 5, 2017

steveklabnik Jul 5, 2017

rthomas commented Jul 5, 2017

rthomas commented Jul 11, 2017

steveklabnik commented Jul 11, 2017

rthomas commented Jul 13, 2017

carols10cents commented Jul 17, 2017

steveklabnik commented Jul 17, 2017

bors commented Jul 17, 2017

Update docs on Error struct. #29355 #42837

Update docs on Error struct. #29355 #42837

Conversation

rthomas commented Jun 22, 2017

rust-highfive commented Jun 22, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthomas commented Jul 3, 2017

rthomas commented Jul 5, 2017

steveklabnik commented Jul 5, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthomas commented Jul 5, 2017

rthomas commented Jul 11, 2017

steveklabnik commented Jul 11, 2017

rthomas commented Jul 13, 2017

carols10cents commented Jul 17, 2017

steveklabnik commented Jul 17, 2017

bors commented Jul 17, 2017