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 url_serde crate with ser/de support for url::Url type #274

Merged
merged 1 commit into from Jan 28, 2017

Conversation

@mhristache
Copy link
Contributor

mhristache commented Jan 27, 2017

This is fixing #273


This change is Reviewable

Copy link
Member

SimonSapin left a comment

Looks good! A couple small things…

license = "MIT/Apache-2.0"

[dependencies]
serde = ">=0.9, <0.10"

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

This is equivalent to "0.9", so please use that. Same for serde_json below.

```
struct MyStruct {
#[serde(deserialize_with = "url_serde::deserialize",

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

If you tried that this works in your project?

This comment has been minimized.

@mhristache

mhristache Jan 27, 2017

Author Contributor

Yes, it does. I would like to add a test case for it but it will need rust nightly until rust 1.15 is released. Is it ok to have a test that run on nightly only?

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

We want to keep running the rest of the tests on stable Rust on Travis. We could add a Cargo feature and enable it based on $TRAVIS_RUST_VERSION, but I think it’s not worth the bother. I ok with just the non-automated test that you did.


/// Serialises `value` with a given serializer.
///
/// This is useful to serialize Hyper types used in structure fields or

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

Replace Hyper with rust-url

/// Serialises `value` with a given serializer.
///
/// This is useful to serialize Hyper types used in structure fields or
/// tuple members with `#[serde(serialize_with = "hyper_serde::serialize")]`.

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

Replace hyper_serde with url_serde

}


#[test]

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

Please add a (cd url_serde && cargo test) line after (cd idna && cargo test) in Makefile, so that this test is run on Travis-CI. (Note that the indentation needs to be an actual tab character, not spaces.)

authors = ["The rust-url developers"]

description = "Serde support for URL types"
documentation = "http://servo.github.io/rust-url/url/index.html"

This comment has been minimized.

@SimonSapin

SimonSapin Jan 27, 2017

Member

Please change the documentation URl to https://docs.rs/url_serde/

@mhristache
Copy link
Contributor Author

mhristache commented Jan 27, 2017

PR updated with your comments @SimonSapin

@SimonSapin
Copy link
Member

SimonSapin commented Jan 28, 2017

Looks great, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2017

📌 Commit 3feb089 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jan 28, 2017

Test exempted - status

@bors-servo bors-servo merged commit 3feb089 into servo:master Jan 28, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bors-servo added a commit that referenced this pull request Jan 28, 2017
Add url_serde crate with ser/de support for url::Url type

This is fixing #273

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/274)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member

SimonSapin commented Jan 28, 2017

@Ms2ger Ms2ger mentioned this pull request Feb 1, 2017
24 of 24 tasks complete
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

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