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

Refactor URL formatting code into its own module. #11

Merged
merged 1 commit into from
Aug 16, 2014

Conversation

michaelsproul
Copy link
Contributor

I've been attempting to implement Show for Iron's custom URL type, and I want to avoid duplicating all of rust-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 implement Show 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


/// Formatter and serializer for URL username and password data.
pub struct UserInfoFormatter<'a> {
pub username: &'a String,
Copy link
Contributor Author

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.

Copy link
Member

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.

@SimonSapin
Copy link
Member

Could you also move UrlNoFragmentFormatter into the new format module?

@michaelsproul
Copy link
Contributor Author

I've moved the UrlNoFragmentFormatter and used generics to handle the PathFormatter API problem. It now accepts both &[String] and &[&str] 😄.

SimonSapin added a commit that referenced this pull request Aug 16, 2014
Refactor URL formatting code into its own module.
@SimonSapin SimonSapin merged commit ec468b2 into servo:master Aug 16, 2014
@SimonSapin
Copy link
Member

Looks good, thanks!

@michaelsproul michaelsproul deleted the formatter-refactor branch August 17, 2014 06:17
@SimonSapin
Copy link
Member

@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?

@michaelsproul
Copy link
Contributor Author

Yeah, looks like it would work for Iron, no worries.

Do you think the TextWriter and build_string! system offers a significant benefit over using Show and format!? When I wrote the formatting code I imagined that people needing strings could create PathFormatter and UserInfoFormatter objects as required and use them like so:

let string = format!("{}", PathFormatter { path: path });

I worry that introducing more generic formatting code only complicates things further when the Show system is already quite fully featured and widely used.

In Iron at the moment I call the Show::fmt method quite a bit, which is nice to have.

https://github.com/iron/iron/blob/master/src/request/url.rs#L118

@SimonSapin
Copy link
Member

I’m not too sure of the design, which is why it’s not merged yet.

The problem with Show is that you need a type to implement it on. If you want re-usable pieces of formatting, you end up with types like PathFormatter that have no other purpose than to implement Show on them. I don’t really like this. You can have various fmt-like functions that can be called from each other or from a Show impl, but you can not create a fmt::Formatter without Show being involved.

fmt::Formatter happens to implement io::Writer, so another approach is to have functions that take a Writer instead of a TextWriter, and have build_string! use a MemWriter. I have another version of this patch that does this. Do you think it would be preferable?

But that version of build_string!, like format!(), includes a redundant UTF-8 check. Hence TextWriter. I have pushed for Unicode streams in the past, and I really think that Show::fmt should be based on them. I’m hoping that someday it will.

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.

@michaelsproul
Copy link
Contributor Author

Oooh, I am seeing where you're coming from now.

TextWriter seems like a great idea, I prefer it to using Writer. I think your current approach in the textwriters branch is merge-able.

Would it make sense to create a separate project for TextWriter to encourage wide spread adoption? Or could we try to get it added to rust-encoding?

@SimonSapin
Copy link
Member

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 :)

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

Successfully merging this pull request may close these issues.

2 participants