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

Add impl Serialize and Deserialize for Option<Url> #279

Merged
merged 1 commit into from
Feb 19, 2017
Merged

Add impl Serialize and Deserialize for Option<Url> #279

merged 1 commit into from
Feb 19, 2017

Conversation

mhristache
Copy link
Contributor

@mhristache mhristache commented Feb 19, 2017

This commit is adding impl Serialize and Deserialize for Option
which is intended to be used with serde serliaze_with and
deserialize_with for derived ser/deser when an url field is optional.

This commit is also adding some tests for derived Serialize and
Deserialize via serde serialize_with and deserialize_with, as
custom_derive is stable since rust 1.15.


This change is Reviewable

@SimonSapin
Copy link
Member

Looks good! A couple changes…


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


.gitignore, line 4 at r1 (raw file):

Cargo.lock
/.cargo/config
.idea

What’s this? If it’s a file you have locally unrelated to anything in the repo, please use .git/info/exclude instead.


url_serde/src/lib.rs, line 102 at r1 (raw file):

/// Serializes this Option<URL> into a `serde` stream.
impl<'a> Serialize for Ser<'a, Option<Url>> {

I think this can be made generic, please do so:

impl<'a, T> Serialize for Ser<'a, Option<T>> where Ser<'a, T>: Serialize

It can help if/when we add serde support for more url types, like Host.


url_serde/src/lib.rs, line 154 at r1 (raw file):

/// Deserializes this Option<URL> from a `serde` stream.
impl Deserialize for De<Option<Url>> {

Same as above.


Comments from Reviewable

@mhristache
Copy link
Contributor Author

.gitignore, line 4 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

What’s this? If it’s a file you have locally unrelated to anything in the repo, please use .git/info/exclude instead.

When defining a project in Intellij Idea IDE it creates a .idea folder where it stores all the settings. I thought it's useful to ignore it as Idea IDE is becoming more and more popular for Rust development.

That said, I don't mind taking it away.


Comments from Reviewable

@SimonSapin
Copy link
Member

Review status: all files reviewed at latest revision, 3 unresolved discussions.


.gitignore, line 4 at r1 (raw file):

Previously, maximih (Maximilian Hristache) wrote…

When defining a project in Intellij Idea IDE it creates a .idea folder where it stores all the settings. I thought it's useful to ignore it as Idea IDE is becoming more and more popular for Rust development.

That said, I don't mind taking it away.

There’s an unbounded number of IDEs in the world. I think Intellij users should rather use a global (to them) gitignore file:

https://help.github.com/articles/ignoring-files/#create-a-global-gitignore


Comments from Reviewable

@mhristache
Copy link
Contributor Author

url_serde/src/lib.rs, line 102 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

I think this can be made generic, please do so:

impl<'a, T> Serialize for Ser<'a, Option<T>> where Ser<'a, T>: Serialize

It can help if/when we add serde support for more url types, like Host.

I am hitting E0275 (error[E0275]: overflow evaluating the requirement _: std::marker::Sized
) when trying to use the signature above. Any ideas how to avoid it?


Comments from Reviewable

@SimonSapin
Copy link
Member

r+ with the .gitignore change removed.


Review status: all files reviewed at latest revision, 2 unresolved discussions.


url_serde/src/lib.rs, line 102 at r1 (raw file):

Previously, maximih (Maximilian Hristache) wrote…

I am hitting E0275 (error[E0275]: overflow evaluating the requirement _: std::marker::Sized
) when trying to use the signature above. Any ideas how to avoid it?

This might be a rustc bug. I’ve filed rust-lang/rust#39959. In the meantime, leave you impls as-is, we can make them generic later if it’s a bug an when the bug is fixed.


Comments from Reviewable

This commit is adding impl Serialize and Deserialize for Option<Url>
which is intended to be used with serde serliaze_with and
deserialize_with for derived ser/deser when an url field is optional.

This commit is also adding some tests for derived Serialize and
Deserialize via serde serialize_with and deserialize_with, as
custom_derive is stable since rust 1.15.
@mhristache
Copy link
Contributor Author

@SimonSapin .gitignore change was removed

@SimonSapin
Copy link
Member

@bors-servo r+


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@bors-servo
Copy link
Contributor

📌 Commit 599b97d has been approved by SimonSapin

@mhristache
Copy link
Contributor Author

@SimonSapin thanks for your time! Can you also push url_serde 0.1.2 to crates.io? Thanks

@SimonSapin SimonSapin closed this Feb 19, 2017
@SimonSapin SimonSapin reopened this Feb 19, 2017
@SimonSapin SimonSapin merged commit e41a920 into servo:master Feb 19, 2017
@SimonSapin
Copy link
Member

Normally a bot should run CI and merge automatically. But that seems broken at the moment, and I don’t know why. https://travis-ci.org/servo/rust-url/builds/203163348 is green, I merged manually.

0.1.2 is now on crates.io

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.

3 participants