Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add rustfmt rules to the project. #575
Add rustfmt rules to the project. #575
Changes from all commits
84ab5c6
0963de5
c7c4f30
178d39d
895f419
8251f7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Won't reducing from the default
100
to90
cause a lot of wrapping? I already feel a bit constrained with the default 100, also considering that almost everyone uses wide screensThere 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'm using only a laptop=) The default value is 120. I didn't try 100 by I know that with 90, it always fits into GitHub split mode review=)
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.
the default value is 100 -> https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#max_width
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.
That is cool, but isn't stable yet:
rust-lang/rustfmt#3350
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.
Many fields from here are unstable=) It is why it is nightly rust fmt
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.
Hmmm. Does this require users to switch between
nightly
andstable
to runcargo fmt
? The codebase itself builds withstable
.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.
They need to do

cargo +nightly fmt
. But all IDE support a separate channel for rustfmt.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 prefer
Module
. Things get a bit confusing, maybe even unreadable, if we have multiple nested levels - whereas they are most of the times just single lines if its per modulehttps://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#Module%5C%3A
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.
Example from our codebase("Crate"):

Example from our codebase("Module"):

I'm okay with both variants but prefer more "Crate" because I can easy collapse imports from one crate or add a
cfg
flag for whole crate=)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.
the cfg flag can go both ways. For example, it is also likely that a cfg flag will affect a subset of imports within a crate, rather than the entire crate.
Will rustfmt avoid collapsing the import if we have a cfg flag on a specific set of imports from a crate?
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.
Yes, it will not touch
cfg
imports. Also, if you have an "enter" between to import section, it will not merge them into one, but it will collapse and sort all inside each section(the same forcfg
).Example with "enter":

Example without "enter":

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 one is very good
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 cool too, but try is so old that we don't even see it anymore. Anyway, its definitely something we should have