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

Derive encodable for Url and subtypes. #27

Closed
wants to merge 1 commit into from
Closed

Conversation

@reem
Copy link

reem commented Sep 6, 2014

Title says it all. Nice for downstream users.

@reem
Copy link
Author

reem commented Sep 8, 2014

@SimonSapin
Copy link
Member

SimonSapin commented Sep 8, 2014

Assuming this is to to serialize to JSON or something similar, do you really want to use the default implementation based on internal data structures rather than just serialize the URL as a string? (Sorry for the delay, I was on vacation.)

@SimonSapin
Copy link
Member

SimonSapin commented Sep 8, 2014

Should Decodable be implemented too?

@reem
Copy link
Author

reem commented Sep 8, 2014

I think it would probably be best to encode/decode from a url string, but for now deriving would be good just to allow people to serialize and de-serialize data which contains Url.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 9, 2014

How about this?

impl<E, S: serialize::Encoder<E>> serialize::Encodable<S, E> for Url {
    fn encode(&self, encoder: &mut S) -> Result<(), E> {
        encoder.emit_str(self.to_string().as_slice())
    }
}


impl<E, D: serialize::Decoder<E>> serialize::Decodable<D, E> for Url {
    fn decode(decoder: &mut D) -> Result<Url, E> {
        Url::parse(try!(decoder.read_str()).as_slice()).map_err(|error| {
            decoder.error(format!("URL parsing error: {}", error).as_slice())
        })
    }
}
@reem
Copy link
Author

reem commented Sep 9, 2014

That looks good. I must confess that I don't really know much about Encodable/Decodable. How would I actually use that? I know there is a decoder for Json, is there a string-decoder?

Also, it would still be beneficial to derive encodable and decodable for all the sub-structures, I know we use Host in Iron, for instance, and I would still like to be able to encode/decode it.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 9, 2014

http://valve.github.io/blog/2014/08/25/json-serialization-in-rust-part-1/ and http://valve.github.io/blog/2014/08/26/json-serialization-in-rust-part-2/ explain these traits. But it sounds like you’re not actually using them. Why the PR in the first place?

@reem
Copy link
Author

reem commented Sep 9, 2014

Some downstream users of Iron have tried to store Urls in structs they would be to derive encodable/decodable and have had to work around it. I tried to derive encodable/decodable for iron::Url, but since it uses url::Host, it doesn't work.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 9, 2014

Alright. I’d take a PR that adds impls similar to #27 (comment)

SimonSapin added a commit that referenced this pull request Sep 9, 2014
@SimonSapin
Copy link
Member

SimonSapin commented Sep 9, 2014

Pushed the one I had for Url in bfdf809.

@reem
Copy link
Author

reem commented Sep 9, 2014

Cool. I think I will actually end up just writing a similar impl for iron::Url instead. Thanks for talking this through.

@reem reem closed this Sep 9, 2014
@SimonSapin SimonSapin mentioned this pull request Dec 24, 2014
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

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