Implement Serialize and Deserialze for Host

[cbrewster]

This change is 

[SimonSapin]

Looks good! Just some minor details.

---

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

---

Cargo.toml, line 4 at r1 (raw file):

name = "url"
version = "1.3.1"

This PR introduces new functionality, so I think SemVer says the version number should be 1.4.0.

---

src/host.rs, line 61 at r1 (raw file):

#[cfg(feature="serde")]
impl ::serde::Serialize for Host {

Host<S=String> is generic (with a default). I think this impl should be impl<S: AsRef<str>> ::serde::Serialize for Host<S>. For example Host<&str> is used in Url::host.

---

src/host.rs, line 73 at r1 (raw file):

#[cfg(feature="serde")]
impl::serde::Deserialize for Host {

This is probably less important than for Serialize (since Self is used in an owned return value, and so can not contains &str), but while you’re at it please make this impl generic as well: impl<S: From<String>> ::serde::Deserialize for Host<S>.

---

src/host.rs, line 73 at r1 (raw file):

#[cfg(feature="serde")]
impl::serde::Deserialize for Host {

Nit: space before ::serde.

---

Comments from Reviewable

[asajeffrey]

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

---

Cargo.toml, line 4 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

This PR introduces new functionality, so I think SemVer says the version number should be 1.4.0.

I think RFC 1105 allows this as a minor change: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-adding-new-public-items.

---

Comments from Reviewable

[SimonSapin]

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

---

Cargo.toml, line 4 at r1 (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I think RFC 1105 allows this as a minor change: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-adding-new-public-items.

Yes, it’s not a breaking change. That would be version 2.0.0.

Cargo treats 1.3.1 and 1.4.0 the same, the difference is only for communicating to humans. (I’d be fine with just two components in the version number, but SemVer seems to be what the Rust ecosystem settled on.)

---

Comments from Reviewable

[cbrewster]

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

Cargo.toml, line 4 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Yes, it’s not a breaking change. That would be version 2.0.0.

Cargo treats 1.3.1 and 1.4.0 the same, the difference is only for communicating to humans. (I’d be fine with just two components in the version number, but SemVer seems to be what the Rust ecosystem settled on.)

Done.

---

src/host.rs, line 61 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Host<S=String> is generic (with a default). I think this impl should be impl<S: AsRef<str>> ::serde::Serialize for Host<S>. For example Host<&str> is used in Url::host.

Done. I had to also add a Serialize bound.

---

src/host.rs, line 73 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

This is probably less important than for Serialize (since Self is used in an owned return value, and so can not contains &str), but while you’re at it please make this impl generic as well: impl<S: From<String>> ::serde::Deserialize for Host<S>.

Done. I had to also add a Deserialize bound.

---

src/host.rs, line 73 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Nit: space before ::serde.

Done.

---

Comments from Reviewable

[SimonSapin]

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

---

src/host.rs, line 61 at r1 (raw file):

Previously, cbrewster (Connor Brewster) wrote…

Done. I had to also add a Serialize bound.

I was thinking of serializing the result of as_ref(). I hadn’t thought of using a Serialize bound. Since the latter is more generic let’s keep it. But now the AsRef bound is unnecessary, so please remove it.

---

src/host.rs, line 73 at r1 (raw file):

Previously, cbrewster (Connor Brewster) wrote…

Done. I had to also add a Deserialize bound.

Same as above, please remove the From bound as it is unnecessary with the Deserialize one.

---

Comments from Reviewable

[cbrewster]

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

---

src/host.rs, line 61 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

I was thinking of serializing the result of as_ref(). I hadn’t thought of using a Serialize bound. Since the latter is more generic let’s keep it. But now the AsRef bound is unnecessary, so please remove it.

Done.

---

src/host.rs, line 73 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Same as above, please remove the From bound as it is unnecessary with the Deserialize one.

Done.

---

Comments from Reviewable

[SimonSapin]

@bors-servo r+

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[bors-servo]

📌 Commit 2aaa190 has been approved by SimonSapin

[bors-servo]

⚡ Test exempted - status