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.

Sign up for GitHub

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

Jump to bottom

Switch to serde_derive #13582

Closed
wants to merge 2 commits into from
Closed

Switch to serde_derive #13582

wants to merge 2 commits into from

Conversation

nox
Copy link
Contributor

@nox nox commented Oct 4, 2016 •
edited by larsbergstrom
Loading

This change is Reviewable

@highfive
Copy link

highfive commented Oct 4, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/Cargo.toml, components/style/lib.rs
  • @asajeffrey: components/constellation/lib.rs, components/constellation/Cargo.toml
  • @fitzgen: components/devtools_traits/Cargo.toml, components/devtools_traits/Cargo.toml, components/profile/Cargo.toml, components/profile_traits/Cargo.toml, components/profile_traits/Cargo.toml, components/devtools/Cargo.toml, components/devtools/lib.rs, components/profile_traits/lib.rs, components/profile_traits/lib.rs, components/devtools_traits/lib.rs, components/devtools_traits/lib.rs, components/profile/lib.rs
  • @KiChjang: components/script/Cargo.toml, components/script_traits/lib.rs, components/script_traits/lib.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/net/Cargo.toml, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 4, 2016
@highfive
Copy link

highfive commented Oct 4, 2016

warning Warning warning

  • These commits modify net, style, layout, gfx, and script code, but no tests are modified. Please consider adding a test!

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 4, 2016
@nox
Copy link
Contributor Author

nox commented Oct 4, 2016

servo/rust-azure#245
servo/webrender#430

@nox nox added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Oct 4, 2016
@nox
Copy link
Contributor Author

nox commented Oct 4, 2016

serde-deprecated/syntex#94

@nox
Copy link
Contributor Author

nox commented Oct 5, 2016

rust-lang/cargo#3164

@nox nox changed the title (Do not merge) Switch to serde_derive Switch to serde_derive Oct 7, 2016
Manishearth
Manishearth approved these changes Oct 7, 2016
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

r+

@nox
Copy link
Contributor Author

nox commented Oct 7, 2016

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 84700c3 has been approved by Manishearth

@highfive highfive assigned Manishearth and unassigned emilio Oct 7, 2016
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Oct 7, 2016
@nox nox removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Oct 7, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 84700c3 with merge 7bffab1...

bors-servo pushed a commit that referenced this pull request Oct 7, 2016
Auto merge of #13582 - nox:derive, r=Manishearth
7bffab1 
Switch to serde_derive

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

💔 Test failed - linux-rel-css

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 7, 2016
nox added 2 commits October 7, 2016 14:39
@nox
Do not use negative discriminants in BorderStyle
0e0df35 
syn, and thus serde_derive, cannot cope with them.
@nox
Switch to serde_derive
df51772
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 7, 2016
@nox
Copy link
Contributor Author

nox commented Oct 7, 2016

The first commit fixes the bug discovered in dtolnay/syn#31.

Manishearth
Manishearth reviewed Oct 8, 2016
View reviewed changes
components/style/values/specified/mod.rs
@@ -1469,16 +1469,16 @@ pub fn parse_border_width(input: &mut Parser) -> Result<Length, ()> {
// The integer values here correspond to the border conflict resolution rules in CSS 2.1 §
// 17.6.2.1. Higher values override lower values.
define_numbered_css_keyword_enum! { BorderStyle:
"none" => none = -1,
Copy link
Member

Choose a reason for hiding this comment

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

file a bug about undoing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would I do this? The negative values have no reason to exist AFAICT.

Manishearth
Manishearth approved these changes Oct 8, 2016
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

r+

@nox
Copy link
Contributor Author

nox commented Oct 8, 2016

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit df51772 has been approved by Manishearth

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 8, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit df51772 with merge 5492051...

bors-servo pushed a commit that referenced this pull request Oct 8, 2016
Auto merge of #13582 - nox:derive, r=Manishearth
5492051 
Switch to serde_derive

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

💔 Test failed - windows-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 8, 2016
@nox
Copy link
Contributor Author

nox commented Oct 8, 2016

According to @eddyb, this is fixed in a rustup. Closing this PR, will include the switch to serde_derive in the rustup.

@nox nox closed this Oct 8, 2016
@nox nox deleted the derive branch October 8, 2016 11:59
@eddyb
Copy link
Contributor

eddyb commented Oct 8, 2016

I didn't say it was. Only was curious if I was the cause of a new bug. If the context is macros 1.1 then it makes much more sense that the cause would be somewhere in there. Maybe it is fixed already on master, but idk for sure. cc @alexcrichton @nrc

@nox
Copy link
Contributor Author

nox commented Oct 8, 2016

@eddyb Anyway, this is never going to merge without a rustup, and a rustup depends on this change too, so this PR is worthless on its own. :) Opened and tried #13649.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth Manishearth approved these changes

Assignees

@Manishearth Manishearth

Labels
S-tests-failed The changes caused existing tests to fail.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nox @highfive @bors-servo @eddyb @Manishearth @emilio