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

Added a method to serialize a Url to a path with a query string #38

Closed
wants to merge 2 commits into from

Conversation

@gtolle
Copy link

gtolle commented Oct 12, 2014

Intended for use by HTTP clients. The Hyper client currently calls serialize_path() when constructing its GET requests, but this discards the query string. I'm suggesting this new rust-url method so that Hyper can use it.

Review on Reviewable

…use by HTTP clients.
@gtolle
Copy link
Author

gtolle commented Oct 12, 2014

Also, this is my first bit of public/production Rust code, so feel free to suggest changes. I'm still learning.

@@ -35,6 +35,26 @@ impl<'a, T: Str + Show> Show for PathFormatter<'a, T> {
}
}

pub struct PathWithQueryFormatter<'a, T:'a> {
pub path: &'a [T],
pub query: &'a Option<String>,

This comment has been minimized.

@Manishearth

Manishearth Oct 12, 2014

Member

Should this be Option<&'b str> instead?

@gtolle
Copy link
Author

gtolle commented Oct 12, 2014

@Manishearth good suggestion. I updated the PathWithQueryFormatter to take a slice instead of a String, following the same conventions as the password field in UserInfoFormatter. But, I kept the named lifetime as 'a instead of 'b, and realized I don't know enough about Rust named lifetimes to understand how to make it work with 'b instead of 'a.

@Manishearth
Copy link
Member

Manishearth commented Oct 12, 2014

pub struct PathWithQueryFormatter<'a, 'b, T:'a> {
 pub path: &'a [T],
 pub query: Option<&'b str>,
}

should work? I'm not too clear on this either, but if the two lifetimes are the same we may get conflicting lifetime issues while using it (eg if we pass it a static string slice and a non-static array slice)

@Manishearth
Copy link
Member

Manishearth commented Oct 12, 2014

Aside from the lifetime thing (and perhaps the indentation for serialize_path_with_query could be improved), this looks good to me.

I'll wait for @SimonSapin to sign off on this though.

@gtolle
Copy link
Author

gtolle commented Oct 12, 2014

OK, I just tried your change with separate lifetimes and it worked fine, as long as I also change:

impl<'a, 'b, T: Str + Show> Show for PathWithQueryFormatter<'a, 'b, T> {

Happy to push it, but before I do, I wonder why UserInfoFormatter used the same lifetime for both the username and password fields instead of splitting into 'a and 'b.

Also, I'm using emacs rust-mode for indentation, and that's what came out for serialize_path_with_query. If you don't mind, could you show me the indentation you'd prefer? I'll make the change.

@Manishearth
Copy link
Member

Manishearth commented Oct 12, 2014

Huh, you're right. I tested it with using strings of different lifetimes -- it seems to upcast to the smaller lifetime.

@gtolle
Copy link
Author

gtolle commented Oct 12, 2014

Hmm, alright then. Thanks for testing this!

Looking forward to hearing from @SimonSapin.

@pyfisch
Copy link
Contributor

pyfisch commented Nov 21, 2015

Is this PR still relevant? Should I create a new PR with updated code for this function?

@SimonSapin
Copy link
Member

SimonSapin commented Apr 4, 2016

With #176 this will be &url[Position::BeforePath..Postition::AfterQuery].

@SimonSapin SimonSapin closed this Apr 4, 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

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