Truncate the serialization in Url::take_after_path

[emilio]

Fixes #222

I did this under the assumption that extending with an empty segment is a no-op. If we want it to cause something like foo.com/bar//baz, then the bit in path_segments_mut needs to be removed.

r? @SimonSapin

---

This change is

[untitaker]

Empty path segments are used for the eventual trailing slash in paths.

[emilio]

@untitaker: Fair enough, removed that bit.

[untitaker]

Also the testcase I provided in #222 still appears to fail :)

Really apprechiate your work btw, I never got around to do this myself but
wanted to.

On Sun, Sep 18, 2016 at 12:47:10AM -0700, Emilio Cobos Álvarez wrote:

@untitaker: Fair enough, removed that bit.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#223 (comment)

[emilio]

@untitaker: That's because that is_empty() bit, and because I'm stupid and forgot to push :)

[untitaker]

Hmm the AppVeyor build fails!

[untitaker]

(also it probably would make sense to add my testcase to this PR)

[Hoverbear]

LGTM just fix the Appveyor build. :))

[Phrohdoh]

This also fixes #227.

@emilio If you don't mind would you also apply this:

diff --git a/tests/unit.rs b/tests/unit.rs
index 2617178..5d3494b 100644
--- a/tests/unit.rs
+++ b/tests/unit.rs
@@ -288,3 +288,14 @@ fn append_empty_segment_then_mutate() {
     url.assert_invariants();
     assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?a=b");
 }
+
+#[test]
+/// https://github.com/servo/rust-url/issues/227
+fn extend_query_pairs_then_mutate() {
+    let mut url: Url = "http://localhost:6767/foo/bar".parse().unwrap();
+    url.query_pairs_mut().extend_pairs(vec![ ("auth", "my-token") ].into_iter());
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?auth=my-token");
+    url.path_segments_mut().unwrap().push("some_other_path");
+    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar/some_other_path?auth=my-token");
+}

I do not know when assert_invariants should be used so I did not include it.

[emilio]

@Hoverbear: The AppVeyor build fails at parsing the first URL in that test (/), because path_to_file_url_segments_windows expects a disk prefix or otherwise returns an error.

I "fixed" it by conditionally compiling that test. I'm pretty sure that failure has nothing to do with these changes (I doubt it passed CI in the first place?).

[emilio]

Yeah, indeed all the closed PRs since the addition of that test fail on AppVeyor.

[SimonSapin]

@bors-servo r=hoverbear+simonsapin

[bors-servo]

📌 Commit 4d4f1a3 has been approved by hoverbear+simonsapin

[bors-servo]

⚡ Test exempted - status