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

Replace unwrap() in doctests with `?` #323

Merged

bors-servo merged 4 commits into servo:master from Enet4:imp/no-unwrap-in-doctests

Jun 13, 2017

Contributor

Enet4 commented May 5, 2017 •

edited

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.

This change is


      Replace unwrap() in doctests with ?

- `ParseError` is handled with the ? operator
- Some other specific results are unwrapped with `expect()`

9846ab1

Member

KiChjang commented May 5, 2017

Why are there so many # symbols added?

Contributor Author

Enet4 commented May 5, 2017

@KiChjang They define lines of code that we want to consider in the tests but hide in the documented output. This is needed here to hide the boilerplate. The code needs to be wrapped in a function that returns a result in order to use the ? operator, but that should be implicit in the examples.

budziq commented May 9, 2017

There seam to be few unwrap calls left in the doctests immediately surrounding the change.
Also, would it not be beneficial to leave the run boilerplate at least once? As url does not provide convenience Result it was actually easier for me look into the source code to find the proper return type than analyze the docs in their current form.

Contributor Author

Enet4 commented May 9, 2017 •

edited

There seam to be few unwrap calls left in the doctests immediately surrounding the change.

Can you specify where? Note that lines hidden from the docs do not need to have their errors captured. Moreover, a good part of the unwraps still around are unwrapping Option, not Result.

Also, would it not be beneficial to leave the run boilerplate at least once? As url does not provide convenience Result it was actually easier for me look into the source code to find the proper return type than analyze the docs in their current form.

That would be debatable. Although a Result alias is not provided, the documented methods do show the error type ParseError, which is exported at the root of the crate. I suppose the crate could consider making the Result alias, although it's just a part of the equation, and not everyone agrees on the same naming conventions.

budziq reviewed May 9, 2017

View changes

budziq left a comment •

edited

Can you specify where?

There are still dome unwrap() calls used in doctest for src/path_segments.rs

Moreover, a good part of the unwraps still around are unwrapping Option, not Result.

Well the point of the referenced lib-blitz issue #312 seams to be centered around promoting correct error handling in rust in general and unwrapping Option is no different from Result in this regard.

That would be debatable.

I completely agree. Just from the perspective of user who has tried to work with url yesterday for the first time I find current examples lacking and I guess that having Result alias would be preferable.

src/path_segments.rs Outdated

  
        /// assert!(url.path_segments_mut().is_err());

        ///

        /// let mut url = Url::parse("http://example.net/foo/index.html").unwrap();

        /// let mut url = Url::parse("http://example.net/foo/index.html")?;

        /// url.path_segments_mut().unwrap().pop().push("img").push("2/100%.png");

budziq May 9, 2017

unwrap() still used in doctest

Enet4 May 9, 2017 •

edited

Author Contributor

That is something I would resolve in another PR: path_segments_mut returns Result<_,()>. With the API like this, we cannot capture it with the ? without mapping the error or rolling a custom error type. Note that using () as error type is not recommended by the Rust API guidelines (C-MEANINGFUL-ERR).

The same issue reproduces across your other mentions.

src/path_segments.rs Outdated

  
            /// # use url::{Url, ParseError};

            /// 

            /// # fn run() -> Result<(), ParseError> {

            /// let mut url = Url::parse("https://github.com/servo/rust-url/")?;

            /// url.path_segments_mut().unwrap().clear().push("logout");

budziq May 9, 2017

unwrap() still used in doctest

src/path_segments.rs Outdated

  
            /// # use url::{Url, ParseError};

            /// 

            /// # fn run() -> Result<(), ParseError> {

            /// let mut url = Url::parse("https://github.com/servo/rust-url/")?;

            /// url.path_segments_mut().unwrap().push("pulls");

budziq May 9, 2017

unwrap() still used in doctest

src/path_segments.rs Outdated

  
            /// url.path_segments_mut().unwrap().push("pulls");

            /// assert_eq!(url.as_str(), "https://github.com/servo/rust-url//pulls");

            ///

            /// let mut url = Url::parse("https://github.com/servo/rust-url/").unwrap();

            /// let mut url = Url::parse("https://github.com/servo/rust-url/")?;

            /// url.path_segments_mut().unwrap().pop_if_empty().push("pulls");

budziq May 9, 2017

unwrap() still used in doctest

src/path_segments.rs Outdated

  
            /// # use url::{Url, ParseError};

            /// 

            /// # fn run() -> Result<(), ParseError> {

            /// let mut url = Url::parse("https://github.com/")?;

            /// let org = "servo";

            /// let repo = "rust-url";

            /// let issue_number = "188";

            /// url.path_segments_mut().unwrap().extend(&[org, repo, "issues", issue_number]);

budziq May 9, 2017

unwrap() still used in doctest

src/path_segments.rs Outdated

  
            /// # use url::{Url, ParseError};

            /// 

            /// # fn run() -> Result<(), ParseError> {

            /// let mut url = Url::parse("https://github.com/servo")?;

            /// url.path_segments_mut().unwrap().extend(&["..", "rust-url", ".", "pulls"]);

budziq May 9, 2017

unwrap() still used in doctest

Contributor Author

Enet4 commented May 9, 2017

For the reasons stated above, I suggest that #299 is resolved before those unwraps are replaced.

dtolnay commented May 12, 2017

Yes the () errors make error handling obnoxious. We would all love a fix for #299.

But as it is, unwrap() is not the way you would handle these in real code. I think the examples should show realistic error handling even if it is awkward, because people will paste these examples into their own code.

dtolnay referenced this pull request May 12, 2017

Open

Replace `()` error types with specific types that implement Error #299

Contributor Author

Enet4 commented May 12, 2017 •

edited

Ok, I will look into them today, possibly devising an error type for the purpose. I also wish to help addressing #299, since it appears we could highly benefit from better error handling.

Update: All right, all unwraps should be gone. I also found a try! in another function's doctests, which I replaced with ?.


      Improve doctests with less unwraps

- Add `CannotBeBaseError` struct to examples involving cannot-be-a-base URLs, map unit errors to this type;
- Replace previous `path_segment().expect()` with `CannotBeBaseError` mapping;
- replace try! with ? in url.with_defaul_port example.

711d6b8

dtolnay commented May 12, 2017

Thanks! That eliminates the remainder of the unwraps.

Some of these have a bit more error-handling noise than I would find valuable. For example the following one from PathSegmentsMut::clear is 12 lines of error handling to 4 lines of example code.

use url::Url;
use std::error::Error;
use std::fmt;

#[derive(Debug)]
struct CannotBeBaseError;
impl fmt::Display for CannotBeBaseError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str(self.description())
    }
}
impl Error for CannotBeBaseError {
    fn description(&self) -> &str { "cannot be a base" }
}

let mut url = Url::parse("https://github.com/servo/rust-url/")?;
url.path_segments_mut().map_err(|_| CannotBeBaseError)?
    .clear().push("logout");
assert_eq!(url.as_str(), "https://github.com/logout");

Since Box<Error> implements From<&str>, what do you think of the following? In contrast to unwrap, this still leaves the user an obvious place to plug in their own error handling after they copy+paste this code. They can't forget to do it like they probably would with unwrap.

use url::Url;

let mut url = Url::parse("https://github.com/servo/rust-url/")?;
url.path_segments_mut().map_err(|_| "cannot be a base")?
    .clear().push("logout");
assert_eq!(url.as_str(), "https://github.com/logout");

Contributor Author

Enet4 commented May 12, 2017

I agree that the current approach is verbose (not to mention that it definitely looks like something that should have been an error variant in the first place!). I'll follow the advice of using &'static str as a placeholder error and push a commit right away.


      Change doctests to use &str as error in some cases

- Remove verbose `CannotBeBase` error in examples, use a `&'static str` instead.

fb71349

dtolnay approved these changes May 12, 2017

View changes

dtolnay left a comment

Great work. Thanks for sticking with this!

dtolnay referenced this pull request May 14, 2017

Merged

Add examples Url methods #309 #340

bjorn3 suggested changes May 16, 2017

View changes

bjorn3 left a comment

run is never called in many doctests, so they are not tested

  
      @@ -44,6 +45,8 @@ assert!(issue_list_url.path_segments().map(|c| c.collect::<Vec<_>>()) ==
    
        assert!(issue_list_url.query() == Some("labels=E-easy&state=open"));

        assert!(issue_list_url.fragment() == None);

        assert!(!issue_list_url.cannot_be_a_base());

        # Ok(())

        # }

bjorn3 May 16, 2017

fn run is not called

  
        let data_url = Url::parse("data:text/plain,Hello?World#").unwrap();

        # fn run() -> Result<(), ParseError> {

bjorn3 May 16, 2017

Same here


      Run the doctests

- add `run().unwrap()` to all result-guarded doctests

c58678a

Contributor Author

Enet4 commented May 16, 2017

@bjorn3 Oops, you are right! I appended calls to run() in the latest commit.

bjorn3 approved these changes May 16, 2017

View changes

Member

SimonSapin commented Jun 13, 2017

This looks great. Thanks all!

Contributor

bors-servo commented Jun 13, 2017

📌 Commit c58678a has been approved by SimonSapin

Contributor

bors-servo commented Jun 13, 2017

⌛️ Testing commit c58678a with merge 923c02f...

bors-servo added a commit that referenced this pull request Jun 13, 2017


      Auto merge of #323 - Enet4:imp/no-unwrap-in-doctests, r=SimonSapin

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

923c02f

Contributor

bors-servo commented Jun 13, 2017

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing 923c02f to master...

bors-servo merged commit `c58678a` into servo:master Jun 13, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed

Details

homu Test successful

Details

bors-servo referenced this pull request Jun 13, 2017

Closed

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

Enet4 deleted the Enet4:imp/no-unwrap-in-doctests branch Jun 13, 2017

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

You can’t perform that action at this time.