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

Add rustfmt rules to the project. #575

Merged
merged 6 commits into from
Sep 7, 2022
Merged

Add rustfmt rules to the project. #575

merged 6 commits into from
Sep 7, 2022

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Aug 22, 2022

Added rustfmt rules. I used my favorite rules but we can discuss your wishes=)
It requires usage of nightly rustfmt because it contains many new configurations.
Formatted the whole project, so it will make ongoing PR harder(you need to fix conflicts)
Added CI to check the fmt.

@xgreenx xgreenx self-assigned this Aug 22, 2022
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 22, 2022

You can check what the code looks like and decide whether it is okay or not=)
And maybe better to merge other PR first before this one.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 23, 2022

We actually can remove it because it is very fas check(~10 seconds) =)

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

We actually had a custom rustfmt config, then removed it in #22. Generally, using any config that isn't the default is a bit iffy, since it invites bikeshedding: some people prefer one thing, other prefer another. By keeping everything at default as a rule, there's no bikeshedding possible on individual preferences.

Copy link
Contributor

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

I think its a good thing to have a fmt standard. But then if we are diverging from the default of rustfmt, we might want to enforce that to all our repos so we don't end up having different styles across the org

@@ -0,0 +1,64 @@
max_width = 90 # changed
Copy link
Contributor

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 to 90 cause a lot of wrapping? I already feel a bit constrained with the default 100, also considering that almost everyone uses wide screens

Copy link
Collaborator Author

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=)

Copy link
Contributor

Choose a reason for hiding this comment

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

.rustfmt.toml Outdated Show resolved Hide resolved
wrap_comments = false
format_code_in_doc_comments = false
comment_width = 80
normalize_comments = true # changed
Copy link
Contributor

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

Copy link
Collaborator Author

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

Copy link
Contributor

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 and stable to run cargo fmt? The codebase itself builds with stable.

Copy link
Collaborator Author

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.
изображение

where_single_line = false
imports_indent = "Block"
imports_layout = "Vertical" # changed
imports_granularity = "Crate" # changed
Copy link
Contributor

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 module

https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#Module%5C%3A

Copy link
Collaborator Author

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=)

Copy link
Member

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.

use crate::{ThingA, ThingB}
#[cfg(feature = "feature-c")]
use crate::ThingC

Will rustfmt avoid collapsing the import if we have a cfg flag on a specific set of imports from a crate?

Copy link
Collaborator Author

@xgreenx xgreenx Aug 31, 2022

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 for cfg).

Example with "enter":
image

Example without "enter":
image

.rustfmt.toml Outdated Show resolved Hide resolved
.rustfmt.toml Outdated Show resolved Hide resolved
fn_args_layout = "Tall"
brace_style = "SameLineWhere"
control_brace_style = "AlwaysSameLine"
trailing_semicolon = false # changed
Copy link
Contributor

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

edition = "2021" # changed
version = "One"
merge_derives = true
use_try_shorthand = true # changed
Copy link
Contributor

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

.rustfmt.toml Outdated Show resolved Hide resolved
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 23, 2022

I think its a good thing to have a fmt standard. But then if we are diverging from the default of rustfmt, we might want to enforce that to all our repos so we don't end up having different styles across the org

Yea, if we are okay to apply rustfmt for fuel-core then better to do the same for all rust repos=)

@adlerjohn
Copy link
Contributor

If this requires an org-wide change then it should have org-wide buy-in. Might be time to follow-up on FuelLabs/rfcs#8

@leviathanbeak
Copy link
Contributor

Reviewed all the config options, apart from maybe imports_granularity = "Crate/Module" I don't see much use in enforcing other ones, that is, diverging from the default values.

Voxelot
Voxelot previously approved these changes Sep 5, 2022
Copy link
Member

@Voxelot Voxelot left a comment

Choose a reason for hiding this comment

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

lgtm, but since this is a big change plz wait for at least one other approval before merging.

Dentosal
Dentosal previously approved these changes Sep 5, 2022
Copy link
Member

@Dentosal Dentosal left a comment

Choose a reason for hiding this comment

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

lgtm as well

@Voxelot
Copy link
Member

Voxelot commented Sep 5, 2022

@adlerjohn we need to update the ci requirements on the repo to merge this pr

@Voxelot
Copy link
Member

Voxelot commented Sep 6, 2022

@xgreenx can you also update CONTRIBUTING.md with the updated instructions for running the nightly formatter?

@xgreenx xgreenx dismissed stale reviews from Dentosal and Voxelot via cf5146d September 6, 2022 23:08
Voxelot
Voxelot previously approved these changes Sep 6, 2022
@xgreenx xgreenx merged commit 142fc32 into master Sep 7, 2022
@xgreenx xgreenx deleted the feature/apply-rustfmt branch September 7, 2022 08:19
@xgreenx xgreenx mentioned this pull request Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants