Add examples Url methods #309 #340

ghost · May 14, 2017

Fix #309

This change is

ghost · May 14, 2017

cc @dtolnay and @SimonSapin

dtolnay · May 14, 2017

dtolnay reviewed May 14, 2017

View changes

dtolnay · May 14, 2017

src/lib.rs

@@ -861,6 +886,20 @@ impl Url {
    }

    /// Return this URL’s query string, if any, as a percent-encoded ASCII string.


Let's show a third example where percent-encoding is necessary.

dtolnay · May 14, 2017

src/lib.rs

+    ///
+    /// let url = Url::parse("https://example.com/api/versions?page=2").unwrap();
+    ///
+    /// assert_eq!(url.path(), "/api/versions");


I think this would be more readable at a glance if the asserts were grouped with the previous line:

use url::Url; let url = Url::parse("https://example.com/api/versions?page=2").unwrap(); assert_eq!(url.path(), "/api/versions"); let url = Url::parse("https://example.com").unwrap(); assert_eq!(url.path(), "/");

dtolnay · May 14, 2017

src/lib.rs

+    /// use url::Url;
+    ///
+    /// let parse_options = Url::options();
+    /// let base = parse_options.parse("https://example.net/api/").unwrap();


Please come up with an example that requires ParseOptions. This one would just use Url::parse.

dtolnay · May 14, 2017

src/lib.rs

+    /// {
+    ///     url.set_fragment(Some("cell=4,1-6,2"));
+    /// }
+    /// assert_eq!(url.fragment(), Some("cell=4,1-6,2"));


Please add an assert on url.as_str() after each of these to show what the full url looks like.

dtolnay · May 14, 2017

src/lib.rs

+    /// {
+    ///     url.set_query(Some("page=2"));
+    /// }
+    /// assert_eq!(url.query(), Some("page=2"));


Please add an assert on url.as_str() after this.

dtolnay · May 14, 2017

src/lib.rs

+    /// use url::Url;
+    ///
+    /// let mut url = Url::parse("http://example.com").unwrap();
+    /// {


This nesting is not needed.

dtolnay · May 14, 2017

src/lib.rs

+    /// {
+    ///     url.set_ip_host("127.0.0.1".parse().unwrap());
+    /// }
+    /// assert_eq!(url.host_str(), Some("127.0.0.1"));


Please add an assert on url.as_str() after this.

dtolnay · May 14, 2017

src/lib.rs

+    ///
+    /// let mut url = Url::parse("mailto:rms@example.com").unwrap();
+    /// let result = url.set_ip_host("127.0.0.1".parse().unwrap());
+    /// assert!(result.is_err());


Since the error type isn't meaningful, please add a brief comment here reminding why this is an error.

dtolnay · May 14, 2017

src/lib.rs

+    ///
+    /// let mut url = Url::parse("mailto:rmz@example.com").unwrap();
+    /// let result = url.set_username("user1");
+    /// assert!(result.is_err());


Please add a comment saying why this is an error.

dtolnay · May 14, 2017

src/lib.rs

+    /// let mut url = Url::parse("ftp://:secre1@example.com").unwrap();
+    /// let result = url.set_username("user1");
+    /// assert!(result.is_ok());
+    /// assert_eq!(url.username(), "user1");


Please add an assert on url.as_str().

dtolnay · May 14, 2017

Nice work. One more comment that applies to all of these: the examples should show realistic error handling. In real code you probably would not want any of these unwrap() calls to crash your application. Use ? instead - see #323 for some examples.

brson · Jun 3, 2017

Ping @hngnaig

ghost · Jun 5, 2017

I just fix code, but i missed test for method path, i will update soon, let me know if any wrong. Thanks @dtolnay.

ghost · Jun 5, 2017

I need help about function path, as doc, method will return &str, that percent-encoded ASCII string. But i assert function shown as bellow, that no percent-encoded.

use url::Url;
let url = Url::parse("https://example.com/foo/bar/baz").unwrap();
assert_eq!(url.path(), "/foo/bar/baz");

I didn't really understand what it is. I think i should assert as bellow.

use url::Url;
use url::percent_encoding;
let url = Url::parse("https://example.com/foo%21bar%21baz").unwrap();
let encoding_path = percent_encoding::percent_encode(b"/foo/bar/baz", PATH_SEGMENT_ENCODE_SET);
let path_str = encoding_path.collect::<String>();
assert_eq!(url.path(), path_str);

Thanks for help me.

SimonSapin · Jun 13, 2017

SimonSapin reviewed Jun 13, 2017

View changes

SimonSapin · Jun 13, 2017

src/lib.rs

+    ///
+    /// # Examples
+    ///
+    /// Get default `ParseOption`, then change base url


ParseOptions is plural. (s missing.)

SimonSapin · Jun 13, 2017

src/lib.rs

+    ///
+    /// let options = Url::options();
+    /// let api = Url::parse("https://api.example.com").ok();
+    /// let base_url = options.base_url(api.as_ref());


Please replace .ok() with .unwrap() and api.as_ref() with Some(&api), to show more typical usage.

Actually, according to the other examples (and as highlighted by @dtolnay), we should not be using unwrap() either. Let's use ? instead, thus forcing users to handle the errors properly.

SimonSapin · Jun 13, 2017

@hngnaig Your examples for path happen not to use percent-encoding. If you’d like to add an example that does, try something like:

let url = Url::parse("https://example.com/Été hiver/").unwrap();
assert_eq!(url.path(), "/%C3%89t%C3%A9%20hiver");

No need to use percent_encode explicitly in that example.

SimonSapin · Jun 19, 2017

Your Url::path example looks good, thanks!

However, I’m not sure how it happened, but this PR is now adding 115 new commits. Please squash them into one commit. (Unless you think it’s valuable to split your changes into multiple commits, but they should not duplicate unrelated changes.)

As an aside, I recommend this git alias to visualize the commit graph, I use it a lot. In ~/.gitconfig, add:

[alias]
graph = log --oneline --graph --all --decorate

Now the git graph command shows branch in your repository.

Now, the usual way to squash is git rebase -i <some-base-commit> (-i is also --interactive) and using the fixup instruction in the editor that comes up. But this might be tricky with so many commits and conflicts, so try this instead:

Check out your branch if it isn’t already.
Use git graph above or some other way to find which commit of the master branch your branch is up-to-date with. As far as I can tell this should be d19d5d0, but double-check.
Run git reset --soft <that-commit>, for example git reset --soft d19d5d0. This changes your branch to be "at" that commit, but keeps all of the checked out files intact.
Run git commit. Unfortunately with this method you will need to rewrite the commit message.
Run git push -f. -f is also --force, it will overwrite the remote branch (in your GitHub fork) regardless of what was there before, even if this effectively removes commits. This should be use very carefully, but it is needed in this case.

ghost · Jun 21, 2017

Thanks @SimonSapin for helping me. I squashed all commit, and reset them at the last commit d19d5d. However, there are still errors, I'm trying to check the code and fix the errors.

SimonSapin · Jun 21, 2017

I can take over the git wrangling if you’d like. Let me know.

ghost · Jun 21, 2017

💯 After trying to fix the error, pull request has been completely done, cc @SimonSapin 💯

KiChjang · Jun 21, 2017

@hngnaig Unfortunately your PR still contains merge commits and isn't properly squashed.

SimonSapin · Jun 21, 2017

It also has unrelated whitespace changes in fuzz/fuzzers/parse.rs and percent_encoding/lib.rs

ghost · Jun 21, 2017

cc @KiChjang, @SimonSapin i squashed all commit, and remove all whitespace in fuzz/fuzzers/parse.rs and percent_encoding/lib.rs

Enet4 · Jun 21, 2017

Enet4 reviewed Jun 21, 2017

View changes

Enet4 · Jun 21, 2017

src/lib.rs

+    ///
+    /// # fn run() -> Result<(), ParseError> {
+    /// let mut url = Url::parse("http://example.com")?;
+    /// url.set_ip_host("127.0.0.1".parse().unwrap());


Is this unwrap acceptable here (I'm asking because 127.0.0.1 is guaranteed to work)? If not, we could change the return type to Result<(), Box<Error>> and map errors into a &'static str.

I think it's acceptable because 127.0.0.1 is valid static ip address. But it will be much better if change return type to Result<(), Box<Error>>.

fn run() -> Result<(), Box<Error>> { url.set_ip_host("127.0.0.1".parse()?); }

SimonSapin · Jun 21, 2017

Thanks you!

@bors-servo r+

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

src/lib.rs, line 1605 at r3 (raw file):

Previously, Enet4 (Eduardo Pinho) wrote…

Is this unwrap acceptable here (I'm asking because 127.0.0.1 is guaranteed to work)? If not, we could change the return type to Result<(), Box<Error>> and map errors into a &'static str.

In normal code I think it’s fine to use .unwrap() when we know that it won’t panic (unless some code is buggy), for example when the input is fixed like here. In the spirit of #312 however we should probably avoid it in examples. Let’s do this later in a follow-up.

Comments from Reviewable

bors-servo · Jun 21, 2017

📌 Commit dd1f232 has been approved by SimonSapin

bors-servo · Jun 21, 2017

⌛️ Testing commit dd1f232 with merge 1807848...

bors-servo · Jun 21, 2017

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

Squash commit

Loading status checks…

dd1f232

servo/rust-url

Join GitHub today

Add examples Url methods #309 #340

Conversation

ghost commented May 14, 2017 • edited by SimonSapin

This comment has been minimized.

ghost commented May 14, 2017

dtolnay reviewed May 14, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dtolnay commented May 14, 2017

This comment has been minimized.

brson commented Jun 3, 2017

This comment has been minimized.

ghost commented Jun 5, 2017 • edited by ghost

This comment has been minimized.

ghost commented Jun 5, 2017

SimonSapin reviewed Jun 13, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Jun 13, 2017

This comment has been minimized.

SimonSapin commented Jun 19, 2017

This comment has been minimized.

ghost commented Jun 21, 2017

This comment has been minimized.

SimonSapin commented Jun 21, 2017

This comment has been minimized.

ghost commented Jun 21, 2017 • edited by ghost

This comment has been minimized.

KiChjang commented Jun 21, 2017

This comment has been minimized.

SimonSapin commented Jun 21, 2017

This comment has been minimized.

ghost commented Jun 21, 2017

Enet4 reviewed Jun 21, 2017 View changes

This comment has been minimized.

Enet4 Jun 21, 2017 • edited

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Jun 21, 2017

This comment has been minimized.

bors-servo commented Jun 21, 2017

This comment has been minimized.

bors-servo commented Jun 21, 2017

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

This comment has been minimized.

bors-servo commented Jun 21, 2017

Hide details View details bors-servo merged commit dd1f232 into servo:master Jun 21, 2017 3 of 4 checks passed

3 of 4 checks passed

ghost commented May 14, 2017 •

edited by SimonSapin

dtolnay reviewed May 14, 2017

View changes

ghost commented Jun 5, 2017 •

edited by ghost

SimonSapin reviewed Jun 13, 2017

View changes

ghost commented Jun 21, 2017 •

edited by ghost

Enet4 reviewed Jun 21, 2017

View changes

Enet4 Jun 21, 2017 •

edited

bors-servo merged commit `dd1f232` into servo:master Jun 21, 2017
3 of 4 checks passed