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

Adapted @BurntSushi's blog post to new error handling guide. #28301

Merged
merged 1 commit into from Sep 10, 2015

Conversation

Projects
None yet
6 participants
@christopherdumas
Contributor

christopherdumas commented Sep 8, 2015

This was @steveklabnik's idea. Thanks @BurntSushi for the awesome blog post!
r? @steveklabnik

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 8, 2015

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

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 8, 2015

Beat me to it! Part of what was taking me so long was fixing the doc tests. It looks like a lot of yours will fail if you run rustdoc --test on it (which I assume is part of the build process).

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 8, 2015

Oh.

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 8, 2015

Hmm. Yeah. 41 fail. That's strange.

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 8, 2015

@christopherdumas It's because of the way I setup the code. The code is itself defined in the article, but it is extracted and stitched together to generate full Rust programs, which collectively form a Cargo package. This alleviated me from having a bunch of # fn some_func_defined_earlier_but_used_again() { ... } everywhere. I was adding those in, but it looks like the task has fallen to you. :-)

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 8, 2015

@christopherdumas And you'll probably have to ignore a lot of the tests at the end in the case study since they use external crates.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 8, 2015

Okay, so this is relevant, but this is why I said it shouldn't take too much editing: https://users.rust-lang.org/t/the-rust-programming-language-is-going-to-be-published-by-no-starch-press/2777

TL;DR: we're gonna get the book published, so I will have a Real Editor reading over it at some point, so we can take care of any problems then. 🎊

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 8, 2015

Ok. But I'll still be looking at it to see if theres an easier way to get the tests to run.

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
chapter will explore those stumbling blocks and demonstrate how to use the
standard library to make error handling concise and ergonomic.
## Table of Contents

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

One question is: do we leave this in?

This comment has been minimized.

@BurntSushi

BurntSushi Sep 8, 2015

Member

The wording may not be appropriate, but I am a fan of delivering a key message somewhere early on: Rust does error handling with return values, not exceptions. However... When I wrote the blog post, I assumed some familiarity with Rust and at least one other programming language, so that message is useful is establishing context. I'm not exactly sure how to calibrate that to TRPL.

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

I'm more thinking that this chapter having its own TOC is unusual; it is helpful here, but also, more sub-headings in rustbook might be a good thing as well. I dunno :/

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

(I do think that that message should be delivered early too)

This comment has been minimized.

@BurntSushi

BurntSushi Sep 8, 2015

Member

Oh! I misunderstood. I very much defer to your judgment on the ToC. :-)

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
@@ -1,308 +1,2147 @@
% Error Handling
# Error Handling
Like most programming languages, Rust encourages the programmer to handle

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

we usually have a space between headings and the body

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
one piece at a time so that you'll come away with a solid working knowledge of
how everything fits together.
When done naively, error handling in Rust can be verbose and annoying. This

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

naïvely

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
Rust uses two terms to differentiate between two forms of error: failure, and
panic. A *failure* is an error that can be recovered from in some way. A
*panic* is an error that cannot be recovered from.
To "unwrap" something in Rust is to say, "Give me the result of the

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

We currently use curly quotes in the book. If you don't want to convert these, I can send a PR to your PR :)

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
is very wrong. Wrong enough that we can't continue with things in the current
state. Another example is using the `unreachable!()` macro:
This code uses [pattern
matching](http://doc.rust-lang.org/1.0.0-beta.5/book/patterns.html) to do *case

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

this link needs to be updated.

also, generally, links get moved to the [][] style in the book, would make this particular paragraph much nicer to read, for example

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
Indeed, `map` is
[defined as a
method](http://doc.rust-lang.org/std/option/enum.Option.html#method.map)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

this should be a relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
the case when an `Option` value is `None`. For example, maybe your program
assumes that the extension of a file is `rs` even if none is present. As you
might imagine, the case analysis for this is not specific to file
extensions---it can work with any `Option<T>`:

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

this should be instead of ---

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
This will give us an error:
(Note that `unwrap_or` is
[defined as a
method](http://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap_or)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
on `Option<T>` in the standard library, so we use that here instead of the
free-standing function we defined above. Don't forget to check out the more
general
[`unwrap_or_else`](http://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap_or_else)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
0.95
The `Option` type has many other combinators
[defined in the standard
library](http://doc.rust-lang.org/std/option/enum.Option.html). It is a good

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
### The `Result` type
The `Result` type is also
[defined in the standard library](http://doc.rust-lang.org/std/result/):

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
Just like `Option`, the `Result` type also has an
[`unwrap` method
defined](http://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
[definition for `Option::unwrap`](#code-option-def-unwrap),
except it includes the error value in the `panic!` message. This makes
debugging easier, but it also requires us to add a
[`Debug`](http://doc.rust-lang.org/std/fmt/trait.Debug.html)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
OK, let's move on to an example.

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

there's an extra \n there

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
in our function and let the caller decide what to do. This means changing the
return type of `double_number`. But to what? Well, that requires looking at the
signature of the
[`parse` method](http://doc.rust-lang.org/std/primitive.str.html#method.parse)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
`FromStr`](http://doc.rust-lang.org/std/primitive.i32.html)
(do a `CTRL-F` in your browser for "FromStr")
and look at its [associated
type](http://doc.rust-lang.org/1.0.0-beta.5/book/associated-types.html) `Err`.

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

updated link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
could (and probably should) also make our function generic, but let's favor
explicitness for the moment. We only care about `i32`, so we need to
[find its implementation of
`FromStr`](http://doc.rust-lang.org/std/primitive.i32.html)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
and look at its [associated
type](http://doc.rust-lang.org/1.0.0-beta.5/book/associated-types.html) `Err`.
We did this so we can find the concrete error type. In this case, it's
[`std::num::ParseIntError`](http://doc.rust-lang.org/std/num/struct.ParseIntError.html).

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

relative link

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
that affect only the error type, such as
[`map_err`](http://doc.rust-lang.org/std/result/enum.Result.html#method.map_err)
(instead of `map`) and
[`or_else`](http://doc.rust-lang.org/std/result/enum.Result.html#method.or_else)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

alll of these links need to be relative

@steveklabnik

View changes

src/doc/trpl/error-handling.md Outdated
uses `ParseIntError` so that we don't have to write it out all the time.
The most prominent place this idiom is used in the standard library is with
[`io::Result`](http://doc.rust-lang.org/std/io/type.Result.html). Typically,

This comment has been minimized.

@steveklabnik

steveklabnik Sep 8, 2015

Member

okay I thought there weren't so many of these, but there are, so I'm just gonna stop :)

This comment has been minimized.

@christopherdumas

christopherdumas Sep 9, 2015

Contributor

I have found all the links that should be relative, and fixed them. :-)

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 10, 2015

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

OK, I've added curly quotes.

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 10, 2015

Okay! I think this is good to go. We just need

  1. the llvm change still needs to go
  2. squash it down

r=me afterwards!

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@steveklabnik What does r=me mean?

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 10, 2015

@christopherdumas For squashing, I think you want to rebase your branch against master. Usually what I do is (with your branch checked out and master up to date) git rebase -i master. Then squash all the commits into one. Then it should give you an opportunity to rewrite the commit message for one commit with all your changes.

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@BurntSushi I tried to do this, but it couldn't apply one of the commits, (the one with the merge conflict). Any advice?

christopherdumas
@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

OK, I squashed all of the commits!

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@BurntSushi @steveklabnik Were ready to merge this!

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

🎊

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@BurntSushi is this command correct? @bors=@steveklabnik

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 10, 2015

@christopherdumas Incidentally, I am not well versed in the language of bors. Best to wait for @steveklabnik to come and get it squared away. :-)

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

Is there a manual for bors? It seems like having one would be a good idea.

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

I think my command was correct, but because I'm not a collaborator or an owner, bors ignored me.

@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 10, 2015

@christopherdumas Well, only people with access can wield the power of bors.

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

Can you try my command? You have access...

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented Sep 10, 2015

@bors r=steveklabnik

@bors

This comment has been minimized.

Contributor

bors commented Sep 10, 2015

📌 Commit e181226 has been approved by steveklabnik

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 10, 2015

@bors: r+

On Sep 10, 2015, at 12:49, Christopher Dumas notifications@github.com wrote:

@BurntSushi @steveklabnik Were ready to merge this!


Reply to this email directly or view it on GitHub.

@bors

This comment has been minimized.

Contributor

bors commented Sep 10, 2015

📌 Commit e181226 has been approved by steveklabnik

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 10, 2015

Yup! Bors only listens to people who have permissions. It's useful because if another committer came along, they could authorize it on my behalf.

On Sep 10, 2015, at 12:49, Christopher Dumas notifications@github.com wrote:

@BurntSushi @steveklabnik Were ready to merge this!


Reply to this email directly or view it on GitHub.

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

@steveklabnik is there a manual for bors yet?

bors added a commit that referenced this pull request Sep 10, 2015

Auto merge of #28301 - christopherdumas:intergrate_error_burnstushi, …
…r=steveklabnik

This was @steveklabnik's idea. Thanks @BurntSushi for the awesome blog post!
r? @steveklabnik
@bors

This comment has been minimized.

Contributor

bors commented Sep 10, 2015

⌛️ Testing commit e181226 with merge 98eeded...

@bors bors merged commit e181226 into rust-lang:master Sep 10, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@BurntSushi

This comment has been minimized.

Member

BurntSushi commented Sep 10, 2015

🎉 Nice work @christopherdumas! :D

@christopherdumas

This comment has been minimized.

Contributor

christopherdumas commented Sep 10, 2015

Thanks! And thank you for the help, and the great blog post!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment