-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
run rustfmt on libsyntax_ext/deriving folder #34113
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
cc @nrc |
if ty_params.is_empty() | ||
&& attr::contains_name(&annitem.attrs, "derive_Copy") => { | ||
if ty_params.is_empty() && | ||
attr::contains_name(&annitem.attrs, "derive_Copy") => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation is pretty strange. Why is the continuation line at the same level is if
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bug, I filed rust-lang/rustfmt#1099 for this
@nrc I'm not sure how to review these patches. They all seem to have parts that look like they might want to be tweaked. With the actual style conventions in flux, are we happy to land these with formatting that is sometimes not very attractive? What's the goal with rustfmting the compiler? |
☔ The latest upstream changes (presumably #34424) made this pull request unmergeable. Please resolve the merge conflicts. |
Triage: no change since my previous comment. |
@brson I would like to not block patches like this on perfect formatting. I would block if we lose formatting that won't easily be recreated (e.g., a matrix alignment) or if the formatting is really gross (i.e., you would be unhappy working with such code), c.f, slightly off (such as the alignment here). Basically, I think rustfmt is going to get better and these things will naturally get better over time, but if we don't get started, we never will. Given that, do you think this PR just needs a rebase to land, or are there other blocking issues with it? If there, it would be nice (but not essential) to file rustfmt issues and close this PR until rustfmt is fixed. |
@nrc I'm fine landing this one, though I guess @srinivasreddy will need to redo the patch at this point. Sorry for the delay @srinivasreddy. |
@bors r+ |
📌 Commit 9652fcb has been approved by |
Thanks @srinivasreddy |
⌛ Testing commit 9652fcb with merge 3773770... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
@bors: retry On Tue, Jul 19, 2016 at 9:06 PM, bors [email protected] wrote:
|
⌛ Testing commit 9652fcb with merge a07fdcd... |
💔 Test failed - auto-win-msvc-64-cargotest |
@bors: retry On Tue, Jul 19, 2016 at 11:14 PM, bors [email protected] wrote:
|
⌛ Testing commit 9652fcb with merge 08638de... |
💔 Test failed - auto-win-gnu-32-opt |
@bors: retry On Wed, Jul 20, 2016 at 5:46 AM, bors [email protected] wrote:
|
run rustfmt on libsyntax_ext/deriving folder
No description provided.