Skip to content

/ rust-url

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 (de)serialization of internal representation, to avoid re-parsing #259

Merged
merged 2 commits into from Jan 23, 2017
Merged

Add (de)serialization of internal representation, to avoid re-parsing #259

merged 2 commits into from Jan 23, 2017
+120 −28

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Dec 19, 2016 •
edited by larsbergstrom

 + random drive-by fixes.

The trick used here to avoid a complex Deserialize impl (either hand-written on in a cumbersome generated file) is to use existing impls, namely those of Option and IpAddr.

@Manishearth, another change from your PR is checking invariants unconditionally (including release mode) so there there is no need for separate (de)serialize_unsafe methods. Does this run-time cost sound acceptable?

r? @Manishearth

This change is Reviewable
@erickt
Copy link

erickt commented Dec 19, 2016

Looks good to me from the serde angle.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 19, 2016

☔ The latest upstream changes (presumably #260) made this pull request unmergeable. Please resolve the merge conflicts.
@Manishearth
Copy link
Member

Manishearth commented Dec 19, 2016

so there there is no need for separate (de)serialize_unsafe methods. Does this run-time cost sound acceptable?

@nox disagrees. I am okay with this situation, sort of.
@Manishearth
Copy link
Member

Manishearth commented Dec 19, 2016

r+ otherwise. Good idea.
@nox
Copy link
Member

nox commented Dec 19, 2016 •
edited

I asked for separate methods because otherwise rust-url becomes pretty much useless to deserialize from JSON or TOML in the wild, where no one cares about the internal representation in rust-url.
Edit: and also because in these contexts, I wouldn't trust that not checking invariants and for normalisation have no security implications for some systems etc.
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 19, 2016

IMO serde::Serialize and serde::Deserialize should be used together. By "in the wild", I assume you mean a situation whene one of the two ends is not serde. In these situations, Url::as_str and Url::parse should be used instead.
@SimonSapin SimonSapin force-pushed the serde-as-internal branch from cc63d5e to bb0fdb5 Dec 19, 2016
@nox
Copy link
Member

nox commented Dec 19, 2016

What do you mean? Many people use serde to serialise configuration and whatnot. Our serde support ought to serialise as values which make sense in most contexts of serde. The internal representation isn't one of them. What about formats which don't even have a serialiser?

Also, this is a breaking change.
@SimonSapin SimonSapin force-pushed the serde-as-internal branch from bb0fdb5 to 94e8d76 Dec 19, 2016
@SimonSapin SimonSapin changed the title Serde: serialize internal representation to avoid re-parsing. Add (de)serialization of internal representation, to avoid re-parsing Dec 19, 2016
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 19, 2016

Alright, I’ve changed the PR to leave the existing impls alone and add new inherent methods instead. r? @nox
This was referenced Dec 19, 2016
Closed
Closed
@SimonSapin
SimonSapin reviewed Dec 22, 2016
View changes
Cargo.toml Outdated
@@ -38,4 +39,4 @@ heapsize = {version = ">=0.1.1, <0.4", optional = true}
idna = { version = "0.1.0", path = "./idna" }
matches = "0.1"
rustc-serialize = {version = "0.3", optional = true}
serde = {version = ">=0.6.1, <0.9", optional = true}
serde = {version = "0.8.20", optional = true}

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Dec 22, 2016

Author Member

nox correctly points out that this is a breaking change. @Manishearth why this change? Can we do without it?

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Dec 22, 2016

Member

Because bincode needs 0.8.20 and cargo isn't smart enough to coalesce the versions in a test-only dependency.

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Jan 22, 2017

Author Member

Can we use JSON instead for the test and revert this?

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jan 22, 2017

Member

Go ahead
Manishearth and others added 2 commits Dec 17, 2016
@Manishearth @SimonSapin
Add (de)serialization of internal representation, to avoid re-parsing.
14b87b9 
Fixes #252, closes #254.
@SimonSapin
Remove unused internal method.
Loading status checks…
5f5b943
@SimonSapin SimonSapin force-pushed the serde-as-internal branch from 94e8d76 to 5f5b943 Jan 23, 2017
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 23, 2017

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2017

📌 Commit 5f5b943 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2017

⌛ Testing commit 5f5b943 with merge c0065ff...
bors-servo added a commit that referenced this pull request Jan 23, 2017
@bors-servo
Auto merge of #259 - servo:serde-as-internal, r=Manishearth
Loading status checks…
c0065ff 
Add (de)serialization of internal representation, to avoid re-parsing

 + random drive-by fixes.

The trick used here to avoid a complex `Deserialize` impl (either hand-written on in a cumbersome generated file) is to use existing impls, namely those of `Option` and `IpAddr`.

@Manishearth, another change from your PR is checking invariants unconditionally (including release mode) so there there is no need for separate `(de)serialize_unsafe` methods. Does this run-time cost sound acceptable?

r? @Manishearth

<!-- 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/259)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2017

☀️ Test successful - status-travis
@bors-servo bors-servo merged commit 5f5b943 into master Jan 23, 2017
5 of 6 checks passed
5 of 6 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the serde-as-internal branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@SimonSapin @erickt @bors-servo @Manishearth @nox
You can’t perform that action at this time.