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.

Sign up for GitHub

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

Jump to bottom

Modify examples to not call unwrap #312

Closed
brson opened this issue May 4, 2017 · 4 comments · Fixed by #323
Closed

Modify examples to not call unwrap #312

brson opened this issue May 4, 2017 · 4 comments · Fixed by #323

Comments

@brson
Copy link
Contributor

brson commented May 4, 2017

Several examples in url call unwrap. It's preferable to use ? to avoid teaching bad practices to new Rust coders. Modify the examples to have some hidden scaffolding to set up the Result type and use ? instead.
@brson brson added easy labels May 4, 2017
@brson brson changed the title Modify examples to not call unwrap. Modify examples to not call unwrap May 4, 2017
@brson brson mentioned this issue May 4, 2017
Closed
19 tasks
@Enet4 Enet4 mentioned this issue May 5, 2017
Merged
@luisbg
Copy link

luisbg commented May 20, 2017

This looks like a fun way to read example code, and help with cleanup.

I can work on this if nobody else is.

@luisbg
Copy link

luisbg commented May 21, 2017

Since the code examples are by default wrapped in a fn main() {, a direct switch to using ? will have the example fail in cargo test because () is expected instead of the Result the ? returns.

The way around it is to add a fn foo() -> Result<> { around the example, but this means making the examples a bit bigger than they should be.

Confirming here what is the approach you want to take.

Going to send a WIP patch to clarify what I mean. Let me know what you think.

@luisbg luisbg mentioned this issue May 21, 2017
Closed
@luisbg
Copy link

luisbg commented Jun 3, 2017

I am looking at this again. Will report soon. Sorry for the delay.

bors-servo pushed a commit that referenced this issue Jun 13, 2017
Auto merge of #323 - Enet4:imp/no-unwrap-in-doctests, r=SimonSapin
923c02f 
Replace unwrap() in doctests with `?`

Fixes #312.
 - All results yielding `ParseError` in the doctests should now be handled with the ? operator.
 - Some other specific results are ~unwrapped with `expect()`~ mapped to a placeholder string message, such as in URLs that are not _cannot-be-a-base_. Users are then expected to adjust this mapping depending on their use case. Once #299 is resolved, we'll be able to propagate these normally.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/323)
<!-- Reviewable:end -->
@luisbg
Copy link

luisbg commented Jun 13, 2017

Nice! Thanks @Enet4 😄

@SimonSapin SimonSapin mentioned this issue Jun 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace unwrap() in doctests with ? Enet4/rust-url
2 participants
@brson @luisbg