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

Add Url::path_segments_mut, fix #188 #194

Merged
merged 3 commits into from May 3, 2016
Merged

Add Url::path_segments_mut, fix #188 #194

merged 3 commits into from May 3, 2016

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented May 2, 2016

@sfackler @maximih @jgillich How does this look? Would these new API help for your use cases? Can they be improved?

@nox, please review but don’t land yet.


This change is Reviewable

@sfackler
Copy link
Contributor

sfackler commented May 2, 2016

This looks great!

@mhristache
Copy link
Contributor

mhristache commented May 2, 2016

I think it looks good even though I find this a bit unexpected:

extern crate url;

fn main() {

    let mut my_url = url::Url::parse("http://this/is/a").unwrap();

    let new_url = my_url.join("./test").unwrap();
    println!("{}", new_url.as_str());

    my_url.path_segments_mut().unwrap().push("./test");
    println!("{}", my_url.as_str());
}

Which prints:

http://this/is/test
http://this/is/a/.%2Ftest

where I was expecting http://this/is/a/test so that it's consistent with join() implementation.

Anyhow, it's not a big deal as I can fi it by using my_url.path_segments_mut().unwrap().push("test")

So I am happy with the implementation. Thanks for your effort

@SimonSapin
Copy link
Member Author

SimonSapin commented May 2, 2016

It is by design that PathSegmentsMut::push escapes slashes and does not interpret . or .. specially: it adds exactly one segment, whatever the characters in that segment. The point is not to do the same as join, you can use join for that.

Actually, I wonder what push(".") or push("..") should do. With the code is it currently is, Url::parse(url.as_str()).unwrap() == url would not hold, since . and .. segments are normalized during parsing. @nox, what do you think?

@nox
Copy link
Member

nox commented May 3, 2016

@nox, what do you think?

That's a good question. Maybe it should panic on . and ..?

Btw, r=me for the rest.

@SimonSapin
Copy link
Member Author

SimonSapin commented May 3, 2016

I've added a commit to ignore . and make .. behave like pop()

@SimonSapin SimonSapin force-pushed the path-segments branch from 245ea02 to e66486a May 3, 2016
@SimonSapin
Copy link
Member Author

SimonSapin commented May 3, 2016

@bors-servo r=nox

(IRC)

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2016

📌 Commit e66486a has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2016

Testing commit e66486a with merge d91d175...

bors-servo added a commit that referenced this pull request May 3, 2016
Add Url::path_segments_mut, fix #188

@sfackler @maximih @jgillich How does this look? Would these new API help for your use cases? Can they be improved?

@nox, please review and but don’t land yet.

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

bors-servo commented May 3, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit e66486a into master May 3, 2016
5 of 6 checks passed
5 of 6 checks passed
code-review/reviewable Review in progress: 0 of 3 files reviewed, all discussions resolved
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the path-segments branch May 3, 2016
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

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