Skip to content

Comments

move modules around#8221

Merged
bors-servo merged 9 commits intoservo:masterfrom
ajnirp:8130-reorganise
Nov 3, 2015
Merged

move modules around#8221
bors-servo merged 9 commits intoservo:masterfrom
ajnirp:8130-reorganise

Conversation

@ajnirp
Copy link
Contributor

@ajnirp ajnirp commented Oct 27, 2015

for #8130

Review on Reviewable

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

highfive commented Oct 27, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

@nox
Copy link
Contributor

nox commented Oct 27, 2015

The branch should be rebased, and that merge commit in the middle of nowhere removed from it.

@jdm
Copy link
Member

jdm commented Oct 27, 2015

Oh, I guess #8130 wasn't clear. We don't actually want to remove the code generation related to the type ids. We only want to re-export the generated values from the new module, like this:

pub use dom::bindings::codegen::InheritTypes::*;

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 27, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 28, 2015
@ajnirp
Copy link
Contributor Author

ajnirp commented Oct 28, 2015

@jdm that makes the fix much simpler, thanks.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 28, 2015
@jdm
Copy link
Member

jdm commented Oct 28, 2015

This looks fine, although a find and replace for dom::bindings::codegen::InheritTypes:: -> dom::bindings::inheritance:: would be nice :)

@ajnirp
Copy link
Contributor Author

ajnirp commented Oct 29, 2015

Ah right, sorry, I forgot to (re-)do that.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 29, 2015
@jdm
Copy link
Member

jdm commented Oct 29, 2015

At this point, it looks like all we need is a rebase against master and fixes for the results of ./mach test-tidy before this can merge :)

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 29, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 30, 2015
@ajnirp
Copy link
Contributor Author

ajnirp commented Oct 30, 2015

done! I didn't correct the test-tidy warning about a name being redefined in CodegenRust.py because that is separate from this patch.

@jdm jdm removed the S-needs-rebase There are merge conflict errors. label Oct 30, 2015
@nox
Copy link
Contributor

nox commented Oct 30, 2015

@wenderen That's not separate, given it happens on that specific PR.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 30, 2015
@jdm
Copy link
Member

jdm commented Oct 30, 2015

Travis doesn't like these changes so far.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 30, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 30, 2015

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 30, 2015
@jdm jdm removed the S-needs-rebase There are merge conflict errors. label Nov 3, 2015
@jdm
Copy link
Member

jdm commented Nov 3, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

📌 Commit 425c0b8 has been approved by jdm

@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 Nov 3, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

⌛ Testing commit 425c0b8 with merge daad09d...

bors-servo pushed a commit that referenced this pull request Nov 3, 2015
Auto merge of #8221 - wenderen:8130-reorganise, r=jdm
daad09d 
move modules around

for #8130

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8221)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 425c0b8 into servo:master Nov 3, 2015
@bors-servo bors-servo mentioned this pull request Nov 3, 2015
Merged
@ajnirp ajnirp deleted the 8130-reorganise branch November 4, 2015 05:23
@nox nox mentioned this pull request Nov 6, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jdm jdm

Labels

S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ajnirp @highfive @nox @jdm @bors-servo