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

Tidy: Fix ordering use statements with braces #13205

Merged
merged 2 commits into from Sep 9, 2016
Merged

Conversation

UK992
Copy link
Contributor

@UK992 UK992 commented Sep 8, 2016

This hack fixes #7412 and matches behavior with rustfmt.


This change is Reviewable

@highfive
Copy link

highfive commented Sep 8, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko_selector_impl.rs, components/style/values/computed/basic_shape.rs, components/style/context.rs, components/style/animation.rs, components/style/servo_selector_impl.rs, components/style/stylesheets.rs, components/style/viewport.rs, components/style/values/specified/mod.rs, components/style/gecko_values.rs, components/style/logical_geometry.rs, components/style/values/computed/mod.rs, components/style/restyle_hints.rs, components/style/keyframes.rs, components/style/values/specified/basic_shape.rs, components/style/matching.rs, components/style/gecko_conversions.rs, components/style/values/specified/position.rs
  • @jgraham: components/webdriver_server/lib.rs
  • @wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy/tidy.py
  • @emilio: components/script/dom/webglrenderingcontext.rs
  • @fitzgen: components/devtools/actors/console.rs, components/devtools/lib.rs, components/profile/mem.rs, components/devtools/actors/network_event.rs, components/devtools/actors/inspector.rs, components/profile/time.rs, components/devtools/actors/timeline.rs
  • @KiChjang: components/script/dom/request.rs, components/script/dom/client.rs, components/net_traits/response.rs, components/net_traits/response.rs, components/script/dom/servohtmlparser.rs, components/script/dom/blob.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/uievent.rs, components/net/data_loader.rs, components/script/dom/workerglobalscope.rs, components/script/dom/bindings/error.rs, components/script/dom/htmltextareaelement.rs, components/net/blob_loader.rs, components/script/dom/bindings/proxyhandler.rs, components/script/script_thread.rs, components/net/resource_thread.rs, components/script/dom/worker.rs, components/script/dom/htmlbuttonelement.rs, components/script/dom/node.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/canvasgradient.rs, components/script/dom/serviceworkercontainer.rs, components/script/dom/range.rs, components/script/webdriver_handlers.rs, components/script/dom/bindings/inheritance.rs, components/script/devtools.rs, components/script/dom/xmlhttprequest.rs, components/net/file_loader.rs, components/script/dom/htmlinputelement.rs, components/script/dom/domimplementation.rs, components/net/image_cache_thread.rs, components/script/parse/html.rs, components/script/layout_wrapper.rs, components/net/http_loader.rs, components/script/dom/bindings/trace.rs, components/script/origin.rs, components/net/filemanager_thread.rs, components/script/dom/bindings/global.rs, components/script/dom/treewalker.rs, components/script/dom/window.rs, components/script/dom/bluetoothremotegattserver.rs, components/script/dom/document.rs, components/script/dom/element.rs, components/script_layout_interface/message.rs, components/script/dom/url.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/htmlelement.rs, components/script/network_listener.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/bindings/callback.rs, components/net/about_loader.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/net/websocket_loader.rs, components/script/dom/domparser.rs, components/script/dom/bluetooth.rs, components/script/dom/bluetoothremotegattdescriptor.rs, components/script/dom/bluetoothremotegattcharacteristic.rs, components/script/dom/htmliframeelement.rs, components/script_layout_interface/wrapper_traits.rs, components/script/dom/websocket.rs
  • @asajeffrey: components/webdriver_server/lib.rs, components/constellation/constellation.rs, components/constellation/pipeline.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 8, 2016
@UK992 UK992 changed the title Tidy sort Tidy: Fix ordering use statements with braces Sep 8, 2016
@wafflespeanut
Copy link
Contributor

It's actually a nice hack, so to speak. Will look at the entire PR later today! :)

@wafflespeanut wafflespeanut assigned wafflespeanut and unassigned glennw Sep 8, 2016
if prev_use:
current_use_cut = current_use.replace("{self,", ".").replace("{", ".")
prev_use_cut = prev_use.replace("{self,", ".").replace("{", ".")
if indent == current_indent and prev_use and current_use_cut < prev_use_cut:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just merge this into the previous block as

if indent == current_indent and current_use_cut < prev_use_cut:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wafflespeanut
Copy link
Contributor

wafflespeanut commented Sep 8, 2016

The changes look good! r+ after addressing my comment. And, while you are at it, can you please change your commit message? "Fix tidy" looks rather confusing, since it doesn't actually fix tidy. It fixes the sorting of use statements reported by tidy 😄

@UK992 UK992 force-pushed the tidy-sort branch 3 times, most recently from 424c547 to 3984039 Compare September 8, 2016 16:43
@UK992
Copy link
Contributor Author

UK992 commented Sep 8, 2016

done.

@wafflespeanut
Copy link
Contributor

Thanks! :)

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 3984039 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 Sep 8, 2016
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #13058) 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 Sep 9, 2016
@wafflespeanut
Copy link
Contributor

Sorry I didn't prioritize this first. Please rebase it this once, and I'll make sure it lands first! :)

@wafflespeanut
Copy link
Contributor

@bors-servo p=129358

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 9, 2016
@UK992
Copy link
Contributor Author

UK992 commented Sep 9, 2016

rebased.

@wafflespeanut
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 93a103b 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. S-needs-rebase There are merge conflict errors. labels Sep 9, 2016
@wafflespeanut
Copy link
Contributor

Looks like this is next to #13193. We can only hope that the other PR doesn't break yours.

@wafflespeanut
Copy link
Contributor

@bors-servo r+ p=1231451 force

@bors-servo
Copy link
Contributor

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

📌 Commit 93a103b has been approved by Wafflespeanut

@bors-servo
Copy link
Contributor

⌛ Testing commit 93a103b with merge 3117787...

bors-servo pushed a commit that referenced this pull request Sep 9, 2016
Tidy: Fix ordering use statements with braces

This hack fixes #7412 and matches behavior with rustfmt.

<!-- 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/13205)
<!-- Reviewable:end -->
@wafflespeanut
Copy link
Contributor

Yay! There we go! 😄

@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@wafflespeanut
Copy link
Contributor

Now, we've broken everything 😐

@UK992 UK992 deleted the tidy-sort branch January 26, 2017 22:43
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.

Import ordering tidy check confused by multiple use statements from same module
5 participants