-
Notifications
You must be signed in to change notification settings - Fork 339
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
Refactor URL formatting code into its own module. #11
Conversation
|
||
/// Formatter and serializer for URL username and password data. | ||
pub struct UserInfoFormatter<'a> { | ||
pub username: &'a String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the API here. In many ways a &str
is neater, but it implies we should switch to Option<&str>
for the password field, and &[&str]
for the path field in PathFormatter
.
The path is what worries me, the shortest expression I have to convert a Vec<String>
to a &[&str]
is this...
path.iter().map(|s| s.as_slice()).collect::<Vec<&str>>().as_slice()
I expected this to fail the borrow-checker, but it doesn't. Makes me wonder if some sort of map_slice()
method would be useful on vectors, as I imagine this is a common pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn’t seem worth the bother.
Could you also move |
I've moved the |
Refactor URL formatting code into its own module.
Looks good, thanks! |
@michaelsproul Here is another possible refactoring for #15, not merged yet: https://github.com/servo/rust-url/compare/textwriters What do you think? Would this work for Iron? |
Yeah, looks like it would work for Iron, no worries. Do you think the let string = format!("{}", PathFormatter { path: path }); I worry that introducing more generic formatting code only complicates things further when the In Iron at the moment I call the https://github.com/iron/iron/blob/master/src/request/url.rs#L118 |
I’m not too sure of the design, which is why it’s not merged yet. The problem with
But that version of So I don’t know. It’s not easy to strike a balance between being in harmony with the ecosystem as it exists now, and pulling it in the direction is think is beneficial by showing the way. |
Oooh, I am seeing where you're coming from now.
Would it make sense to create a separate project for |
rust-encoding has something similar already. I’d like to do some work on the rust-encoding API to make it as general as possible (including Unicode streams) and then unify. But you know, only so many hours every day :) |
I've been attempting to implement
Show
for Iron's custom URL type, and I want to avoid duplicating all ofrust-url
's formatting code.I've made two formatters publicly available in their own module, for URL paths and userinfo (usernames + passwords). This should allow people writing their own extensions to
rust-url
to implementShow
for their types.I wasn't quite sure where to put the tests, but they seem quite happy in a little sub-module. I figured the
tests
module was better suited to parsing/library tests.Related: iron/iron#130