Skip to content

Run rustfmt on build_helper #49360

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

Merged
merged 1 commit into from
Apr 13, 2018

Conversation

topecongiro
Copy link
Contributor

@topecongiro topecongiro commented Mar 25, 2018

Using rustfmt 0.4.1-nightly (e784712 2018-04-09).

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2018
@varkor
Copy link
Member

varkor commented Mar 25, 2018

I really don't think this is an improvement. There are some rough conventions in rustc that these changes are inconsistent with (e.g. alignment for wrapped expressions). rustfmt is not the final word when it comes to correctness.

I don't necessarily think we want strict style guidelines, but perhaps there should be some statement regarding reformatting, because this sort of pull request doesn't seem to be uncommon, but I think blindly running rustfmt throughout rustc is not the right thing to do.

@sinkuu
Copy link
Contributor

sinkuu commented Mar 28, 2018

Isn't "conventions in rustc" what rustfmt just used to have without consensus, and is being revised by formatting RFCs? This repository have been applied rustfmt some years ago: https://github.com/rust-lang/rust/pulls?page=3&q=is%3Apr+run+rustfmt+is%3Aclosed&utf8=%E2%9C%93

@bors
Copy link
Collaborator

bors commented Apr 1, 2018

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

@topecongiro
Copy link
Contributor Author

I thought the idea was to eventually replace the current tidy check with rustfmt, so incrementally running rustfmt is good, but this is not true anymore?

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton Apr 2, 2018
@nrc
Copy link
Member

nrc commented Apr 4, 2018

I thought the idea was to eventually replace the current tidy check with rustfmt, so incrementally running rustfmt is good, but this is not true anymore?

'replace tidy' is a bit complicated because it does stuff like the license check and iirc some test-specific stuff. However, we absolutely do want to be running rustfmt on the compiler and on CI, so landing PRs like this is desirable. (In the past they've been hard to review and land due to conflicts, so it might be better to just close the tree and reformat the world some day, but it is useful to find out how well rustfmt it performing in the meantime).

@nrc
Copy link
Member

nrc commented Apr 4, 2018

@bors: r+

@bors
Copy link
Collaborator

bors commented Apr 4, 2018

📌 Commit f5d0961 has been approved by nrc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2018
@bors
Copy link
Collaborator

bors commented Apr 9, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 9, 2018
@topecongiro topecongiro force-pushed the run-rustfmt/build_helper branch from f5d0961 to 23b880a Compare April 12, 2018 05:49
@topecongiro
Copy link
Contributor Author

Rebased to resolve the conflict.

@nrc
Copy link
Member

nrc commented Apr 13, 2018

@bors: r+

@bors
Copy link
Collaborator

bors commented Apr 13, 2018

📌 Commit 23b880a has been approved by nrc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2018
@bors
Copy link
Collaborator

bors commented Apr 13, 2018

⌛ Testing commit 23b880a with merge eec3208...

bors added a commit that referenced this pull request Apr 13, 2018
Run rustfmt on build_helper

Using rustfmt 0.4.1-nightly (e784712 2018-04-09).
@bors
Copy link
Collaborator

bors commented Apr 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing eec3208 to master...

@bors bors merged commit 23b880a into rust-lang:master Apr 13, 2018
@topecongiro topecongiro deleted the run-rustfmt/build_helper branch April 13, 2018 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants