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

Remove all uses of unwrap from our docs #34

Closed
sgrif opened this issue Nov 30, 2015 · 9 comments
Closed

Remove all uses of unwrap from our docs #34

sgrif opened this issue Nov 30, 2015 · 9 comments

Comments

@sgrif
Copy link
Member

sgrif commented Nov 30, 2015

It's a terrible habit, and I agree that we shouldn't have it in our docs. I've gotten rid of a decent number recently, but I'd like to eliminate the rest in favor of try! (this will take some annoying boilerplate in our doctests, but it's probably worth it).

@masklinn
Copy link

How do you figure that'd work for examples using main e.g. second example of the readme?

@defyrlt
Copy link

defyrlt commented Nov 30, 2015

I think that .unwrap() is a great thing for docs/examples. You don't have to teach your users how to handle error cases - there's a chapter in the Book for it.

@sgrif
Copy link
Member Author

sgrif commented Nov 30, 2015

The example using main is probably fine. I think many of our examples could change to use try!, or change their assertion to compare against Ok though

@sgrif
Copy link
Member Author

sgrif commented Nov 30, 2015

I don't think we need to teach users how to handle error cases, but if we can avoid unwrap without actually making the example less clear, I'd like to.

@mfpiccolo
Copy link
Contributor

If this repo is going to be a popular place for people who newer to Rust (which I think it will be) it is important to show how to do it the right way. Or don't show it at all.

Perhaps saving the Result as a variable and then a comment underneath about dealing with the result?

@hiimtaylorjones
Copy link

Shouldn't this be closed because of the above PR? If not, I'm more than willing to finish what work wasn't covered by #45!

@sgrif
Copy link
Member Author

sgrif commented May 10, 2016

I think that there are still a few places left but I can't confirm at the moment. Feel free to let me know if that's the case

@hiimtaylorjones
Copy link

hiimtaylorjones commented May 12, 2016

Ah, I see what you're talking about.

Upon doing a global project search for /// assert_eq!(, I've found that cases that use .unwrap() in the assert are all using Ok([statement]) now. Is this the desired beahvior?

Anyways, I'm recommending closing this issue and my Issue #34 PR if everything is as it should be. Sorry for the misunderstanding!

@sgrif sgrif closed this as completed May 12, 2016
@sgrif
Copy link
Member Author

sgrif commented May 12, 2016

Thanks!

On Thu, May 12, 2016 at 7:31 AM Taylor Jones notifications@github.com
wrote:

Ah, I see what you're talking about.

Upon doing a global project search for /// assert_eq!(, I've found that
cases that use .unwrap() in the assert are all using Ok([statement]) now.
Is this the desired beahvior?

Anyways, I'm closing this PR and recommending that #34
#34 be closed if everything
is as it should be. Sorry for the misunderstanding!


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#34 (comment)

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

No branches or pull requests

5 participants