Simple formatting of some modules #24927
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
☔ The latest upstream changes (presumably #24967) made this pull request unmergeable. Please resolve the merge conflicts. |
| pub fn default_options() -> Vec<RenderOption> { vec![] } | ||
| pub fn default_options() -> Vec<RenderOption> { | ||
| vec![] | ||
| } |
There was a problem hiding this comment.
I think you should also support the style of function as it was before.
There was a problem hiding this comment.
I do like having the whole fn sig and body in one line when the body really is short. Is this hard to accommodate, @nrc ? Or is this more of a philosophical thing about "bodies should be presented separately from signatures" ?
There was a problem hiding this comment.
It's a bit of both tbh, I do prefer function bodies always having their own line - makes it easier to read and distinguish provided from required methods. It was also the easiest thing to do.
|
Overall this looks reasonable. There were some nits noted above. @nrc maybe you should do another pass with your fixed |
| fn with_no_errors<T, F>(&mut self, f: F) -> T where | ||
| F: FnOnce(&mut Resolver) -> T, | ||
| fn with_no_errors<T, F>(&mut self, f: F) -> T | ||
| where F: FnOnce(&mut Resolver) -> T |
There was a problem hiding this comment.
I don't like this where placement. It's doesn't scale well to multiple lines in the where clause.
fn with_no_errors<T, F>(&mut self, f: F) -> T where
F: FnOnce(&mut Resolver) -> T,
T: MoreBoundsHere,Mostly the question is: What does the more complicated where clause look like. Bunched up on one line the docs is not acceptable, we need readable & diffable :-)
There was a problem hiding this comment.
It scales to:
fn with_no_errors<T, F>(&mut self, f: F) -> T where
where F: FnOnce(&mut Resolver) -> T,
T: MoreBoundsHere,
I find that easier to scan because you see the where keyword when scanning down the lhs
There was a problem hiding this comment.
I think it's ok.
Are we free to choose any style even if dumb editors may make it hard to replicate? I guess rustfmt can fix it. I'm sure I can get Vim to do this style, but it's a tedious style to space out manually for those with dumber editors or those that just don't install rust plugins.
In most places in mk/tests.mk, it's positioned after rpass-full and before cfail-full (because rfail comes before cfail). The order of tests seems a little inconsistent, but reordering everywhere would obscure this commit.
|
@pnkfelix rebased and, as suggested, manual fixups separated out better |
| fn escape_char<F>(c: char, mut f: F) where F: FnMut(char) { | ||
| fn escape_char<F>(c: char, mut f: F) | ||
| where F: FnMut(char) | ||
| { |
There was a problem hiding this comment.
This feels strange since the curly braces normally open at the end of the function signature, but here it's on it's own line. I understand why it's done, but there's a definite aesthetic dissonance.
|
☔ The latest upstream changes (presumably #25171) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing due to inactivity, but looking forward to possibly using rustfmt in the future! |
Done automatically with rustfmt.