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

Feature: Add cargo fmt check to github action #87

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Conversation

gschup
Copy link
Owner

@gschup gschup commented Nov 7, 2023

We already do this in the ggrs crate, I feel it should be done here, too.

@@ -22,3 +22,5 @@ jobs:
run: cargo build --verbose
- name: Run tests
run: cargo test --verbose
- name: Check formatting
run: cargo fmt --check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's what I've been doing in my crates:

      - name: Fmt
        run: cargo fmt --all -- --check

Haven't checked what the difference is

Copy link
Owner Author

@gschup gschup Nov 7, 2023

Choose a reason for hiding this comment

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

From cargo fmt --help:

--all
Format all packages, and also their local path-based dependencies

Since this repo only has a single package and no local dependencies, it should not make a difference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So i guess it will probably only be relevant if we want to add derive suport

"Rollback Entities have checksum {:X}",
result.0
);
trace!("Rollback Entities have checksum {:X}", result.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this. I kept tripping this change while I was working on the other PRs and kept reverting it since it wasn't a part of my current work!

@bushrat011899
Copy link
Contributor

A good change! Should clippy be added as well? Currently it doesn't flag any issues, but it's definitely helped me during development.

@gschup
Copy link
Owner Author

gschup commented Nov 7, 2023

While I love working with clippy, I am not sure it is correct to fail the build if clippy has something. Some of the warnings are not always necessary and I am not the biggest fan of putting clippy ignore annotations in the code.

@gschup gschup merged commit 44cca88 into main Nov 7, 2023
1 check passed
@gschup gschup deleted the feature/fmt_check branch November 7, 2023 11:54
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