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

[WIP] Modify examples to not call `unwrap` #345

Closed
wants to merge 1 commit into from
Closed

Conversation

@luisbg
Copy link

luisbg commented May 21, 2017

This is for issue #312

Just fixing one case to check this is what the issue is about and that I understand the intended fix.


This change is Reviewable

@luisbg luisbg force-pushed the luisbg:unwrap branch from 11935ac to 752423a May 21, 2017
@behnam
Copy link
Member

behnam commented May 23, 2017

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


src/lib.rs, line 270 at r1 (raw file):

    /// use url::Url;
    ///
    /// fn foo() -> Result<(), url::ParseError> {

If you like to keep the function definition part of the example, then the content of the block should be indented.

But, rustdoc has a nice way of letting you hide these execution details, using # at the beginning of a line.

For example, this part would be:

    /// # fn foo() -> Result<(), url::ParseError> {
    /// ...
    /// # Ok(())
    /// # }

More here: https://doc.rust-lang.org/book/documentation.html#documenting-macros


Comments from Reviewable

@behnam
Copy link
Member

behnam commented May 23, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@luisbg
Copy link
Author

luisbg commented May 23, 2017

Cool!

I will fix this tomorrow, probably. Currently traveling for work meeting so might not have the time to get around to it.

@mgeisler
Copy link
Contributor

mgeisler commented May 30, 2017

I believe you should also include a call to the foo() function -- otherwise the code in question is only compiled, but not actually executed.

@luisbg
Copy link
Author

luisbg commented Jun 3, 2017

I am looking at this again. Thanks for the pointer @mgeisler

Will report soon. Sorry for the delay.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 13, 2017

The latest upstream changes (presumably #323) made this pull request unmergeable. Please resolve the merge conflicts.

@Enet4
Copy link
Contributor

Enet4 commented Jun 13, 2017

#312 has been addressed in #323 (see the latest Url#join for instance). Are there any new examples with unwrap since then?

@SimonSapin
Copy link
Member

SimonSapin commented Jun 13, 2017

This is done in #323. Thanks for the PR anyway!

@SimonSapin SimonSapin closed this Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.