Skip to content

run rustfmt on libsyntax_ext folder - #35614

Merged
bors merged 1 commit into
rust-lang:masterfrom
srinivasreddy:syntax_ext_rustfmt
Aug 17, 2016
Merged

run rustfmt on libsyntax_ext folder#35614
bors merged 1 commit into
rust-lang:masterfrom
srinivasreddy:syntax_ext_rustfmt

Conversation

@srinivasreddy

@srinivasreddysrinivasreddy commented Aug 12, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@rust-highfive

rust-highfive commented Aug 12, 2016

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

retep998
retep998 reviewed Aug 12, 2016
View reviewed changes
Comment threadsrc/libsyntax_ext/format.rs
prec),
self.ecx.field_imm(sp,
self.ecx.ident_of("width"),
width)]);

@retep998retep998Aug 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is horrifying.

@srinivasreddysrinivasreddyAug 12, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That was the first impression i got too. Let's see what @nrc says .

@nrcnrcAug 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's kind of edge-casey because the better formatting requires breaking after the ( which is usually avoided. You could add a skip annotation to the statement, or just ignore it - I'm willing to eat the occasional poor formatting in exchange for automated formatting everywhere.

@NercuryNercuryAug 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not make a rule where for every broken ( the appropriate ending ) is also broken?
That way it would look visually consistent; would solve this edge case.

@nrcnrcAug 14, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's not really the problem. The problem is that we have no heuristics for when to break after the ( in the first place.

@strega-nilstrega-nilAug 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we switch to a style that doesn't have these kinds of awful edge cases? These continue to pop up. We have an alternative (finally), based on winapi:

https://ubsan.github.io/style/

@jseyfriedjseyfriedAug 16, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ubsan Wow, thanks for writing that up -- I really like that style!

#34584 (comment) summarizes @petrochenkov's and my thoughts on these PRs. I really dislike rustfmt's non-block indenting ("visual indenting") that makes this example so horrifying (and also appears to be the main cause of the problems you referenced in the "Why we need this" section).

@eddybeddybAug 17, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The style I prefer is this:

self.ecx.expr_struct(sp, path,vec![self.ecx.field_imm(sp,self.ecx.ident_of("fill"), fill),self.ecx.field_imm(sp,self.ecx.ident_of("align"), align),self.ecx.field_imm(sp,self.ecx.ident_of("flags"), flags),self.ecx.field_imm(sp,self.ecx.ident_of("precision"), prec),self.ecx.field_imm(sp,self.ecx.ident_of("width"), width)]);

EDIT: Apparently this is "block indenting" which is part of @ubsan's style guide.

@strega-nilstrega-nilAug 17, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's block style :)

last argument is multi-line

(note that I just added this; I used to only do that for multi-line single-argument functions, but I realized that this was a failure I needed to address, so I made sure it was all functions which ended with a multi-line argument).

@nikomatsakis

nikomatsakis commented Aug 16, 2016

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Aug 16, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit d652639 has been approved by nikomatsakis

@nikomatsakis

nikomatsakis commented Aug 16, 2016

Copy link
Copy Markdown
Contributor

I'm willing to eat the occasional poor formatting in exchange for automated formatting everywhere.

This is also my POV, though if someone wants to file an issue on rustfmt, that seems fine. (I don't think this PR is the place to debate what rustfmt's heuristics unless there is a genuine bug.)

@strega-nil

strega-nil commented Aug 16, 2016

Copy link
Copy Markdown
Contributor

@nikomatsakis the issue was, until about 3 hours ago, we did not have a team to debate this :P

jseyfried
jseyfried reviewed Aug 16, 2016
View reviewed changes
Comment threadsrc/libsyntax_ext/deriving/generic/ty.rs
vec![self_ty],
lifetimes,
self_params,
Vec::new())

@jseyfriedjseyfriedAug 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function call can remain on one line -- not sure why rustfmt breaks it up...

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 17, 2016
Jonathan Turner
Rollup merge ofrust-lang#35614- srinivasreddy:syntax_ext_rustfmt, r…
336cad4
…=nikomatsakis
run rustfmt on libsyntax_ext folder
@sophiajtsophiajt mentioned this pull request Aug 17, 2016
Closed
bors added a commit that referenced this pull request Aug 17, 2016
@bors
Auto merge of#35743- jonathandturner:rollup, r=jonathandturner
b28ffa5
Rollup of 24 pull requests
- Successful merges: #34370, #35415, #35595, #35610, #35613, #35614, #35621, #35660, #35663, #35670, #35671, #35672, #35681, #35686, #35690, #35691, #35695, #35707, #35708, #35713, #35722, #35725, #35726, #35731
- Failed merges: #35395
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 17, 2016
Jonathan Turner
Rollup merge ofrust-lang#35614- srinivasreddy:syntax_ext_rustfmt, r…
74549f8
…=nikomatsakis
run rustfmt on libsyntax_ext folder
@sophiajtsophiajt mentioned this pull request Aug 17, 2016
Merged
bors added a commit that referenced this pull request Aug 17, 2016
@bors
Auto merge of#35747- jonathandturner:rollup, r=jonathandturner
7ac11ca
Rollup of 23 pull requests
- Successful merges: #34370, #35415, #35595, #35610, #35613, #35614, #35621, #35660, #35663, #35670, #35671, #35672, #35681, #35686, #35690, #35695, #35707, #35708, #35713, #35722, #35725, #35726, #35731
- Failed merges: #35395
@bors
bors merged commit d652639 into rust-lang:masterAug 17, 2016
@srinivasreddy
srinivasreddy deleted the syntax_ext_rustfmt branch August 18, 2016 14:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakisnikomatsakis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@srinivasreddy@rust-highfive@nikomatsakis@bors@strega-nil@eddyb@Nercury@retep998@nrc@jseyfried