Skip to content

/ servo

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

Report lines starting with && in tidy #10751

Merged
merged 3 commits into from Apr 22, 2016
Merged

Report lines starting with && in tidy #10751

merged 3 commits into from Apr 22, 2016

Conversation

@zwn
Copy link
Contributor

zwn commented Apr 20, 2016

Partial implementation of the issue #10692 (the easy part).

This change is Reviewable
@highfive
Copy link

highfive commented Apr 20, 2016

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

highfive commented Apr 20, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/rust_tidy.rs, python/tidy/servo_tidy/tidy.py
@zwn
Copy link
Contributor Author

zwn commented Apr 20, 2016

I see the CI is blocking on tidy returning 0. What should be the next step now? Should I go and clean up the instances? I am reluctant to do that since it will mess up blame (by replacing relevant info with irrelevant indentation changes).
@metajack
Copy link
Contributor

metajack commented Apr 20, 2016

If you change tidy you'll need to make tidy pass as well. You might make git blame slightly harder, but I think we all know how to follow history to get the real blame, so it's not a big concern.
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 20, 2016

And it's not even 20 lines, so shrug
@zwn
Copy link
Contributor Author

zwn commented Apr 21, 2016

Anything else?
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

☔ The latest upstream changes (presumably #10758) made this pull request unmergeable. Please resolve the merge conflicts.
python/tidy/servo_tidy_tests/test_tidy.py
@@ -61,6 +61,7 @@ def test_rust(self):
self.assertEqual('extra space before :', errors.next()[2])
self.assertEqual('use &[T] instead of &Vec<T>', errors.next()[2])
self.assertEqual('use &str instead of &String', errors.next()[2])
self.assertEqual('operators should go at the end of the first line', errors.next()[2])

This comment has been minimized.

Sign in to view
Copy link
@wafflespeanut

wafflespeanut Apr 22, 2016

Member

Now that #10758 has landed, we should ensure that there are no more errors.

This comment has been minimized.

Sign in to view
Copy link
@zwn

zwn Apr 22, 2016

Author Contributor

Done.
zwn added 3 commits Apr 20, 2016
@zwn
Add newline to the output.
01b111c 
The last line of output from tidy did not end with a newline if some
errors were reported.
@zwn
Move '&&' to the end of the previous line.
924d804 
Following #10692 this is just a
formating change to satisfy a new tidy requirement of not having '&&' at
the beginning of a line.
@zwn
Report lines starting with &&.
Loading status checks…
39780ca
@zwn
Copy link
Contributor Author

zwn commented Apr 22, 2016

I've rebased it on current master and reordered the commits so that the check is enabled only after the files had been fixed.
@wafflespeanut
Copy link
Member

wafflespeanut commented Apr 22, 2016

Thanks for doing this! :)

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

📌 Commit 39780ca has been approved by Wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

⌛ Testing commit 39780ca with merge d926b5d...
bors-servo added a commit that referenced this pull request Apr 22, 2016
@bors-servo
Auto merge of #10751 - zwn:tidy-start-operator, r=Wafflespeanut
Loading status checks…
d926b5d 
Report lines starting with && in tidy

Partial implementation of the issue #10692 (the easy part).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10751)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Apr 22, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 39780ca into servo:master Apr 22, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@zwn zwn deleted the zwn:tidy-start-operator branch Apr 22, 2016
@wafflespeanut wafflespeanut mentioned this pull request Apr 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wafflespeanut wafflespeanut

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@zwn @highfive @metajack @Ms2ger @bors-servo @wafflespeanut
You can’t perform that action at this time.