Add new error handling section #674
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| fn setup() { | ||
| // Ignore the return value because we don't care about it. | ||
| let _ = File::create("a") | ||
| .and_then(|mut file| file.write_all(b"grape")); |
There was a problem hiding this comment.
I might be missing important context, but I would be very careful about using let _ = .... Does using unwrap here work? If so, that would be much better.
There was a problem hiding this comment.
👍, let _ is an antipattern imho
On Tue, Dec 1, 2015 at 7:05 PM, Andrew Gallant notifications@github.com
wrote:
In examples/error/option_with_result/combinator_combinations/result_try.rs
#674 (comment)
:@@ -0,0 +1,49 @@
+use std::io::prelude::*;
+use std::fs::File;
+
+type Result = std::result::Result<T, String>;
+
+// Setup to make this work. Create two files with some info.
+fn setup() {
- // Ignore the return value because we don't care about it.
- let _ = File::create("a")
.and_then(|mut file| file.write_all(b"grape"));I might be missing important context, but I would be very careful about
using let _ = .... Does using unwrap here work? If so, that would be much
better.—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust-by-example/pull/674/files#r46359722.
|
@mdinger This looks pretty good! I will say though, that while I read through everything, I had a really hard time evaluating it because it's out of order in Github's diff view. It makes it difficult to see whether the flow of information works. Is there a simple way to view this in its published form? |
|
@BurntSushi I just uploaded it here. It's just a copy so while most things seem correct, the editor syntax highlighting seems to be missing. Syntax highlighting still works properly normally so it's probably that I didn't copy something properly but I'm not sure what. The new error handling section is here. |
|
Thanks. If you see any pages that need a |
| DoubleError::Parse(ref e) => Some(e), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please check my impl of error::Error. I think the above case should be None but wasn't sure.
|
I updated the rendered site as well |
|
@mdinger Thanks for uploading a rendered version of this! It was very helpful. I read through and things LGTM. Nice work. 👍 |
…ound. Recurse into fmt instead of calling to_string
|
Sweet! I committed some more fixes and updated the render. The commit message states what changed. I'm pretty happy with the result. @BurntSushi Thanks. You've been tremendously helpful. |
Add new error handling section
|
Thanks so much! |
|
Awesome! Fastest rbe review for a big PR ever! |
|
As far as I'm concerned, that's due to @BurntSushi :) On Dec 5, 2015, 12:09 -0500, mdingernotifications@github.com, wrote:
|
I added a new error handling section which I based on the @BurntSushi error handling blog doc. In general I think his docs motivate error handling really well however at the end of the docs where they diverge from strings as errors, it wasn't quite as good. You're left wondering why you'd ever bother implementing the
Errortrait andFromandDisplaywhen you only needDisplay. Or perhaps you needFromalso. Anyway, I tried a slightly different approach by trying to approach the error trait and others as basically optional depending on the purpose and needs of the library/binary. He probably said more in the case study but I never actually looked at that part (so it may be absolutely wonderful).In some sense, this may be quite practical because the examples are entirely usable and runnable in comparison to the original. The original, while each is runnable, the playpen links are basically useless which wouldn't work in this case.
I originally didn't like that I had to write the result error handling sections by loading files but after the fact, it seems like a good result and I'm fairly happy with it.
@steveklabnik r?
@BurntSushi If you have any opinions/critiques, please add them.
I'm not sure who else I could cc. I could possibly rename some of the sections and perhaps add some more
See alsos because I might not have added enough but I'm not sure.Sorry it took so long to get this up. I had this mostly to this point for a while but was taking a break and doing other things for a couple weeks I guess.
This branch is currently hosted here. The error handling section is here.