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 format test to CI #1748

Merged
merged 2 commits into from
Jun 24, 2022
Merged

Add format test to CI #1748

merged 2 commits into from
Jun 24, 2022

Conversation

costinsin
Copy link
Contributor

@costinsin costinsin commented Jun 19, 2022

To enforce uniformity for all PRs, the CI checks if the code
is formatted right using cargo fmt tool.

Results after implementing the format test in CicleCI, but before fixing the format errors: https://cirrus-ci.com/build/4684991404703744
Results after fixing the format errors: https://cirrus-ci.com/build/5423803479097344

Solves #770

@costinsin costinsin force-pushed the fmt_testing branch 5 times, most recently from 6498cee to 8a17256 Compare June 19, 2022 13:40
@costinsin costinsin marked this pull request as ready for review June 19, 2022 14:02
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Call me old-fashioned, but I still prefer 80-characters lines. Could you please add a rustfmt.toml file that contains:

max_width = 80

.cirrus.yml Outdated
@@ -18,6 +18,8 @@ build: &BUILD
- . $HOME/.cargo/env || true
- $TOOL +$TOOLCHAIN -Vv
- rustc +$TOOLCHAIN -Vv
- if [ -z "$TOOLCHAIN" ]; then rustup component add rustfmt; else rustup +$TOOLCHAIN component add rustfmt; fi
- $TOOL +$TOOLCHAIN fmt --all -- --check
Copy link
Member

Choose a reason for hiding this comment

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

cargo fmt ignores cfg directives, so running it once suffices to provide coverage for every OS. Running it on every OS is duplicative. Instead, you should either create a new task just for this, or else put it into the "Rust Stable" task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a new CI task. This way, if this is the only test failing, the person who submitted the PR will easily know that the PR failed due to bad formatting. I have tested the commit without applying the formatting changes and everything seems fine

Copy link
Contributor Author

@costinsin costinsin Jun 21, 2022

Choose a reason for hiding this comment

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

I have also added the rustfmt.toml file specifying that the max width needs to be 80 characters.

@costinsin costinsin force-pushed the fmt_testing branch 2 times, most recently from ffaf9f9 to f3da089 Compare June 21, 2022 12:18
@costinsin costinsin requested a review from asomers June 21, 2022 13:08
.cirrus.yml Show resolved Hide resolved
To enforce uniformity for all PRs, the CI checks if the code
is formatted rigth using `cargo fmt` tool.

Signed-off-by: Costin-Robert Sin <[email protected]>
Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit a45e9f8 into nix-rust:master Jun 24, 2022
bors bot added a commit that referenced this pull request Nov 12, 2022
1854: Reformat everything r=rtzoeller a=SUPERCILEX

Unfortunately, #1748 didn't do the trick because of rust-lang/rustfmt#3253. This PR fully enforces global formatting.

Closes #770

Co-authored-by: Alex Saveau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants