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.toml to standardize formatting #2787

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented May 15, 2024

This PR adds a rustfmt.toml which configures rustfmt.
In particular, it specifies formatting for imports grouping:

group_imports = "StdExternalCrate" # create three groups for std, external and local crates
imports_granularity = "Module" # Merge imports from the same module

These options are still unstable, so they need to be run with cargo +nightly fmt ....

For reviewing it's easier to review it by commit instead of as a whole.

  • The first commit adds rustfmt.toml and updates the cargo.sh script to use nightly for cargo fmt.
  • The second commit is the result of running just format.

@jprendes jprendes added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 15, 2024
@jprendes jprendes force-pushed the rustfmt branch 4 times, most recently from 2f76b1c to 45c293d Compare May 15, 2024 13:47
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Comment on lines +47 to +49
components: clippy
- name: Install nightly rustfmt
run: rustup toolchain install nightly --component rustfmt --profile minimal --no-self-update
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we add the nightly directly in the setup rust step? If not can we have a comment here mentioning we use this for formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we add the nightly directly in the setup rust step?

That would result in using nightly clippy, which is not my intention here.
I'm open to suggestions :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I can't think of anything better. We can keep this as is 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 15, 2024

Hey can we add a rev-ignore file like this , and add the second commit to it? That way the blame would not consider the formatting changes. Corresponding git doc : https://github.com/git/git/blob/ae3f36dea16e51041c56ba9ed6b38380c8421816/Documentation/blame-options.txt#L113-L125

Not sure how well the blame would pick up the changes , but given that blender uses it for its formatting commits, it should be good-enough for us to start with this PR. Otherwise the setup commit looks, good, ci passed, so assuming that works 👍

Thanks!

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes
Copy link
Contributor Author

jprendes commented May 15, 2024

The ignore file seems to be working:

Bypassing the ignore file is done appending a ~ to the SHA/branch in the URL.

@jprendes jprendes requested review from YJDoc2 and utam0k May 15, 2024 18:52
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

I fine with the rustfmt and ignore-rev, and assuming that as the CI is passing , formatting is ok. Maybe wait for a look from utam0k .

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

😍

@utam0k
Copy link
Member

utam0k commented May 16, 2024

Hey can we add a rev-ignore file like this , and add the second commit to it? That way the blame would not consider the formatting changes. Corresponding git doc : https://github.com/git/git/blob/ae3f36dea16e51041c56ba9ed6b38380c8421816/Documentation/blame-options.txt#L113-L125

Not sure how well the blame would pick up the changes , but given that blender uses it for its formatting commits, it should be good-enough for us to start with this PR. Otherwise the setup commit looks, good, ci passed, so assuming that works 👍

Thanks!

I didn't know that. Good idea.

@utam0k utam0k merged commit 2b86907 into youki-dev:main May 16, 2024
28 checks passed
@github-actions github-actions bot mentioned this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants