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

Migrate from rustc-serialize to Serde #3682

Merged
merged 1 commit into from
Feb 22, 2017
Merged

Conversation

alexcrichton
Copy link
Member

This commit migrates Cargo as much as possible from rustc-serialize to
Serde. This not only provides an excellent testing ground for the toml
0.3 release but it also is a big boost to the speed of parsing the JSON
bits of the registry.

This doesn't completely excise the dependency just yet as docopt still
requires it along with handlebars. I'm sure though that in time those
crates will migrate to serde!

@rust-highfive
Copy link

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

@brson do you know if there's an easy way to run this through crater? Or should we get it into a nightly and then run crater with that?

I highly suspect that this will break a number of crates in the ecosystem, but I'd love to know how/why ahead of time!

also cc @dtolnay

println!("{}", Json::Object(map));
let mut json: Value = serde_json::to_value(&t).unwrap();
json.as_object_mut().unwrap()
.insert("reason".to_string(), Value::String(t.reason().to_string()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I released index_mut in serde_json 0.9.6:

json["reason"] = json!(t.reason());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah unfortunately that requires an IndexSet trait I think as otherwise this attempts to look up a slot in the json which doesn't exist yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it works 😺

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh the compile error I encountered was because I forgot to update serde_json.

Ahhh clever! I was flabbergasted this worked but when I checked up on the implementation it was a nice trick!

I approve 👍

type Value = TomlVersion;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("an semver version")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a semver version"

@bors
Copy link
Contributor

bors commented Feb 11, 2017

☔ The latest upstream changes (presumably #3668) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 22, 2017

☔ The latest upstream changes (presumably #3733) made this pull request unmergeable. Please resolve the merge conflicts.

This commit migrates Cargo as much as possible from rustc-serialize to
Serde. This not only provides an excellent testing ground for the toml
0.3 release but it also is a big boost to the speed of parsing the JSON
bits of the registry.

This doesn't completely excise the dependency just yet as docopt still
requires it along with handlebars. I'm sure though that in time those
crates will migrate to serde!
@brson
Copy link
Contributor

brson commented Feb 22, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2017

📌 Commit a5a298f has been approved by brson

@bors
Copy link
Contributor

bors commented Feb 22, 2017

⌛ Testing commit a5a298f with merge be0b499...

bors added a commit that referenced this pull request Feb 22, 2017
Migrate from rustc-serialize to Serde

This commit migrates Cargo as much as possible from rustc-serialize to
Serde. This not only provides an excellent testing ground for the toml
0.3 release but it also is a big boost to the speed of parsing the JSON
bits of the registry.

This doesn't completely excise the dependency just yet as docopt still
requires it along with handlebars. I'm sure though that in time those
crates will migrate to serde!
@bors
Copy link
Contributor

bors commented Feb 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: brson
Pushing be0b499 to master...

@sunng87
Copy link
Contributor

sunng87 commented Mar 3, 2017

@alexcrichton seems I'm a little late for this. You can use feature flag serde_type for handlebars so it will turn off rustc_serialize dependency and use serde completely. I can create a patch if you are OK with it.

bors added a commit that referenced this pull request Mar 3, 2017
Use serde type system for handlebars

This will help cargo to drop rustc_serialize as dependency (#3682). Handlebars actually supports using serde_json as its type system instead of rustc_serialize. And I'm planning to drop rustc_serialize in future releases.
@alexcrichton
Copy link
Member Author

@sunng87 was gonna mention here as well that it's definitely welcome, thanks for the PR!

bors added a commit that referenced this pull request Dec 17, 2018
Document `name` and `authors` in [package]

I don't think we need to take a principled stand on the exact meaning of
"authors", so I left it somewhat vague. Also, it was made optional in #3682 so
the existing docs were a little wrong.

Closes #5934
@ehuss ehuss added this to the 1.17.0 milestone Feb 6, 2022
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.

7 participants