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

Fix wording for Option<T>.unwrap #23791

Merged
merged 1 commit into from
Mar 28, 2015
Merged

Fix wording for Option<T>.unwrap #23791

merged 1 commit into from
Mar 28, 2015

Conversation

jviereck
Copy link
Contributor

Fixes #23713.

@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 @nikomatsakis (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 CONTRIBUTING.md for more information.

@nikomatsakis
Copy link
Contributor

@bors r+ 63bbdc1 rollup

@nikomatsakis
Copy link
Contributor

thanks!

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 27, 2015
@apasel422
Copy link
Contributor

This seems overly explicit. Can't this just be

Moves the value out of the option if it is `Some`, or panics if it is `None`.

@jviereck
Copy link
Contributor Author

This seems overly explicit. Can't this just be

You have a point here. You're version sounds cleaner/simplier and I like it. Maybe your proposed description can be extended to still include the v like:

Moves the value `v` out of the option if it is `Some(v)`, or panics if it is `None`.

How does this sound like?

What is the correct procedure to change this PR? Can I just push a new commit into this branch to adjust the docs or should I open a new PR?

@steveklabnik
Copy link
Member

So, normally pushing a new commit would adjust, but since @alexcrichton has started a rollup, I'm not sure.

@alexcrichton
Copy link
Member

Oh oops, sorry about that! If you want to send me a patch I'll put it in the rollup if it bounces, and otherwise you can just make a new PR after this one goes through :)

@bors bors merged commit 63bbdc1 into rust-lang:master Mar 28, 2015
@jviereck jviereck changed the title Fix wording for Option<T>.unwrap. Fixes #23713 Fix wording for Option<T>.unwrap. Mar 30, 2015
@jviereck jviereck changed the title Fix wording for Option<T>.unwrap. Fix wording for Option<T>.unwrap Mar 30, 2015
jviereck added a commit to jviereck/rust that referenced this pull request Mar 30, 2015
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 31, 2015
Based on the comment from @apasel422  in rust-lang#23791 (comment).
Where @apasel422 proposed
```
Moves the value out of the option if it is `Some`, or panics if it is `None`.
```
I include in this PR the version
```
Moves the value `v` out of the `Option` if it is `Some(v)`, or panics if it is `None`.
```
which 
- is a little bit more precise about what value is actually returned
- uses `Option` over just "option" in the part `out of the [Option]

r? @steveklabnik, @apasel422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Option.unwrap documentation more precise
7 participants