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

Ensure crate are alphabetically sorted #8692

Merged
merged 1 commit into from Nov 28, 2015
Merged

Ensure crate are alphabetically sorted #8692

merged 1 commit into from Nov 28, 2015

Conversation

GuillaumeGomez
Copy link
Contributor

#7441

cc @nox

Review on Reviewable

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

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

@highfive
Copy link

warning Warning warning

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

@wafflespeanut
Copy link
Contributor

Review status: 0 of 22 files reviewed at latest revision, 1 unresolved discussion.

python/tidy.py, line 221 [r1] (raw file):
Um, check_rust already checks for lines that begin with mod and use. Why don't we include this as a part of it? That way, we won't be needing another travel through all the files :)

Comments from the review on Reviewable.io

@GuillaumeGomez
Copy link
Contributor Author

@wafflespeanut: Good point, I updated it this way.

@jdm
Copy link
Member

jdm commented Nov 27, 2015

@wafflespeanut Would you like to do a full review for this?

@wafflespeanut
Copy link
Contributor

@jdm I'll be glad :)
@GuillaumeGomez Can you squash the commits together? Because, the latest commit seems to modify the previous one. Squash it and we'll land it :)

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

Comments from the review on Reviewable.io

@wafflespeanut wafflespeanut added S-awaiting-answer Someone asked a question that requires an answer. S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-awaiting-answer Someone asked a question that requires an answer. labels Nov 28, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 28, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 28, 2015

@bors-servo delegate=wafflespeanut

@bors-servo
Copy link
Contributor

✌️ @wafflespeanut can now approve this pull request

@wafflespeanut
Copy link
Contributor

There are some nits (which I might've overlooked earlier), Sorry...

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

python/tidy.py, line 227 [r3] (raw file):
Shall we have this, like

uses, mods = [], []
prev_crate = {}

We're having quite a lot of newlines.

python/tidy.py, line 233 [r3] (raw file):
Shall we have a reasonable name like crate_name?

python/tidy.py, line 245 [r3] (raw file):
Shall we move all those lines below this line, so that we don't need cut_line, and make use of line itself?

Comments from the review on Reviewable.io

@wafflespeanut wafflespeanut 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. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 28, 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 Nov 28, 2015
@wafflespeanut
Copy link
Contributor

@bors-servo r+

Thanks! :)

@wafflespeanut
Copy link
Contributor

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

Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

@bors-servo delegate=Wafflespeanut

@bors-servo
Copy link
Contributor

✌️ @wafflespeanut can now approve this pull request

@wafflespeanut
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 6e7de62 has been approved by Wafflespeanut

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

⌛ Testing commit 6e7de62 with merge dbff1ab...

bors-servo pushed a commit that referenced this pull request Nov 28, 2015
Auto merge of #8692 - GuillaumeGomez:patch-1, r=Wafflespeanut
dbff1ab 
Ensure crate are alphabetically sorted

cc @nox

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

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

@bors-servo bors-servo merged commit 6e7de62 into servo:master Nov 28, 2015
@GuillaumeGomez GuillaumeGomez deleted the patch-1 branch November 28, 2015 15:10
@bors-servo bors-servo mentioned this pull request Nov 28, 2015
Merged
@wafflespeanut wafflespeanut mentioned this pull request Nov 29, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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.

None yet
7 participants
@GuillaumeGomez @highfive @wafflespeanut @jdm @Ms2ger @bors-servo @Manishearth