-
Notifications
You must be signed in to change notification settings - Fork 941
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
Refine rustfmt #2325
Refine rustfmt #2325
Conversation
examples/control_flow.rs
Outdated
use winit::{ | ||
event::{Event, KeyboardInput, WindowEvent}, | ||
event_loop::EventLoop, | ||
window::WindowBuilder, | ||
}; | ||
use winit::event::{Event, KeyboardInput, WindowEvent}; | ||
use winit::event_loop::EventLoop; | ||
use winit::window::WindowBuilder; |
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.
I personally quite like this grouping, are we missing an option?
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.
I dislike it, I find that it makes merge conflicts harder
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.
I've personally switched to imports_granularity = "Item"
in my current personal project to make my life easier on that front, but it does tend to create massive walls of use
statements.
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.
I personally hate the grouping current winit is using, and personally I don't have it in Wayland backend, since rustfmt doesn't enforce it. It just nearly impossible to read it and rebase. Or maybe I've just used to Module
option.
For what it's worth, |
We agreed to do this in today's meeting, though only after a few of the larger in-flight PRs have been merged. I raised the concern that users would have to run
|
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.
I assume the only thing you've changed is rustfmt.toml
. Had a look at it, I agree with all the options, though I think you might get trouble with reorder_impl_items
until madsmtm/objc2#479 is resolved, so might want to wait with that.
The only thing I could change is comment width to |
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.
unstable_features
is missing.
I would also like to add the following:
I'm also against the following rules because they move away from the default Rust style:
rustfmt.toml
Outdated
use_small_heuristics = "Max" | ||
normalize_comments = true | ||
reorder_impl_items = true | ||
use_try_shorthand = true |
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 is deprecated, I don't think we need that in Rustfmt.
rustfmt.toml
Outdated
use_small_heuristics = "Max" | ||
normalize_comments = true | ||
reorder_impl_items = true | ||
use_try_shorthand = true | ||
force_explicit_abi=true |
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 can be removed, its default is already true
.
And |
7e2b7f4
to
8d94895
Compare
Stable rustfmt lacks a lot of features resulting in worse formatted code, thus use nightly formatter.
That's just a prototype how it should look to apply nightly rustfmt with a bit more involved rules.
There's no intent to do that before the new keyboard API rework.