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

Manipulating path segments #188

Closed
SimonSapin opened this issue Apr 21, 2016 · 7 comments
Closed

Manipulating path segments #188

SimonSapin opened this issue Apr 21, 2016 · 7 comments

Comments

@SimonSapin
Copy link
Member

Url currently has pop_path_segments and push_path_segment methods, but they have a design problem reported by @sfackler. A path is at minimum / which is considered like a single path segment that happens to be empty (like trailing slashes in general), so push_path_segment("foo") make the path be //foo with two slashes.

Fixing this may involve breaking API changes, so instead I’ll make these methods private (effectively removing them) before publishing 1.0.0, in order to unblock it. I’ll add them back (or something else to provide this functionality) in 1.1.0.

@sfackler
Copy link
Contributor

In the meantime, what's the right way to add path segments? Something like

let mut url = ...;
url.set_path(&format!("{}/{}", url.path(), percent_encode(new_component.as_bytes(), PATH_SEGMENT_ENCODE_SET);

?

@sfackler
Copy link
Contributor

I guess it would actually need to check if url.path() == "/" to avoid the empty path problem.

@SimonSapin
Copy link
Member Author

Yes, something like that.

@mhristache
Copy link
Contributor

mhristache commented Apr 24, 2016

I guess the current issue is related so I am posting here. Let me know if I should open a new issue.

I am trying to switch to url 1.0.0 but I cannot find the equivalent of the url 0.5 code below:

let mut path = url.path_mut().unwrap();
path.push(segment.to_owned());

I am trying to append a segment to a path which doesn't end with a trailing / and url.join() cannot handle this scenario. Is there a way to do this with url 1.0.0 or I need to do some ugly string concat to make sure a trailing / is always present and then use url.join()?

Thanks

@SimonSapin
Copy link
Member Author

In 1.0.0 you have to use string concatenation and url.set_path(p). In 1.1.0 I want to add something like url.push_path_segment(s) (but maybe a different API) which would do the right thing in various situations.

@jgillich
Copy link
Contributor

jgillich commented Apr 27, 2016

The example by @sfackler doens't appear to work, you'd have to create a String out of url.path() first, which is not great.

src/transport.rs:46:40: 46:43 error: cannot borrow `url` as immutable because it is also borrowed as mutable [E0502]
src/transport.rs:46         url.set_path(&format!("{}/{}", url.path(), path));
                                                           ^~~
<std macros>:2:26: 2:57 note: in this expansion of format_args!
src/transport.rs:46:23: 46:57 note: in this expansion of format! (defined in <std macros>)
src/transport.rs:46:9: 46:12 note: previous borrow of `url` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `url` until the borrow ends
src/transport.rs:46         url.set_path(&format!("{}/{}", url.path(), path));
                            ^~~

@SimonSapin
Copy link
Member Author

SimonSapin commented Apr 27, 2016

Yeah, for now you have to move the format!(…) expression into a separate variable.

The API I have in mind would be something like this:

impl Url {
    fn path_segments_mut(&mut self) -> PathSegmentsMut {}
}
struct PathSegmentsMut<'a> {
    url: &'a mut Url
}
impl<'a> PathSegmentsMut<'a> {
    fn iter(&self) -> PathSegments {}
    fn clear(&mut self) {}
    fn pop(&mut self) {}
    fn push(&mut self, s: &str) {}
}

bors-servo pushed a commit that referenced this issue 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants