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

rustfmt librustc_resolve #34584

Closed
wants to merge 1 commit into from
Closed

rustfmt librustc_resolve #34584

wants to merge 1 commit into from

Conversation

tshepang
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@petrochenkov
Copy link
Contributor

cc @jseyfried, most of this formatting is yours.
My opinion is that this is a good source of issues for rustfmt, but not ready to land because it makes formatting consistently worse.

@tshepang
Copy link
Member Author

tshepang commented Jul 1, 2016

@petrochenkov what does "most of this formatting is yours" mean?

@petrochenkov
Copy link
Contributor

@tshepang
"yours" == @jseyfried's (he maintains these parts of the compiler currently)

@tshepang
Copy link
Member Author

tshepang commented Jul 1, 2016

@which parts of the diff do you not like?

kind: NameBindingKind::Module(self.0),
span: self.1,
vis: self.2,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Struct expression fits into one line but splitted into several lines anyway (1).

Copy link
Member Author

Choose a reason for hiding this comment

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

I find this more readable

@petrochenkov
Copy link
Contributor

Ok, that's enough for now.
My personal approach to code formatters is to start with "change nothing" configuration and enable only transformations with near-zero false-positives (whitespaces before/after punctuation, indentation), line-wrapping heuristics, in particular, are almost never enabled with this approach.
This differs a lot from rustfmt general approach, that's why I avoid looking at rustfmt librustc_xxx PRs at all cost unless they touch my code :D

@jseyfried
Copy link
Contributor

jseyfried commented Jul 3, 2016

@petrochenkov You summed up my thoughts on these PRs exactly :)

Also, "visual indenting" of function calls with many arguments really bothers me:

let foo = some_long_function_name("a reasonable string literal",
                                  another_argument,
                                  a,
                                  b,
                                  c,
                                  x,
                                  y);

This often causes really bad rightward drift, really bad diffs, wasted vertical space, and general ugliness.

I'd much prefer

let foo = some_long_function_name(
    "a relatively short string literal", another_argument, a, b, c, x, y,
);

@bors
Copy link
Contributor

bors commented Jul 10, 2016

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

@tshepang
Copy link
Member Author

highly-opposed and too many merge conflicts

@tshepang tshepang closed this Jul 21, 2016
@tshepang tshepang deleted the rustfmt-librustc_resolvevc branch July 21, 2016 19:29
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.

6 participants