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

Adding cargo fix to pre-commit hook #488

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Feb 9, 2024

Regarding #153

We add the following command to a new pre-push hook:
cargo clippy --all-features -- -W clippy::all -A clippy::style -A clippy::forget_copy -A clippy::forget_ref

The expected behavior is that the push will fail if an error is found during the check.
Since this command is also run in our CI, this can save some time and avoid waiting for a build that will fail.

Ignoring the check

It is possible to run git push --no-verify to avoid running the new hook.

@gianfra-t gianfra-t linked an issue Feb 9, 2024 that may be closed by this pull request
@ebma
Copy link
Member

ebma commented Feb 15, 2024

Thanks for the detailed description @gianfra-t. Although it's just a draft it might have been best to either add all 'devs' as reviewers or ping them in the comments so that this PR doesn't get missed.

About the changes itself, you mention that you are not convinced about the results. What would you consider a dealbreaker here? I agree it's not ideal that apparently it's not possible to run clippy only on the modified files but I think it's fine if it's also run on files that were not modified. Eventually all Rust files in the project should adhere to the same formatting. And your suggestion to add this to a 'pre-push' hook sounds reasonable.

@gianfra-t gianfra-t requested a review from a team February 15, 2024 15:52
@gianfra-t
Copy link
Contributor Author

What would you consider a dealbreaker here?

At least in my computer it just takes a LOT of time for each commit, and I think it may be a bit annoying. To be honest, I would not really activate this hook as is. But I guess it is a matter of personal preference! Let me know if you prefer to try this as is, or attempt the pre-push alternative.

@TorstenStueber
Copy link
Contributor

I also think that pre-push is the better solution.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Let's try a pre-push script then, I tested it locally now and can confirm that it can take quite a while.

.maintain/add-precommit-hook.sh Outdated Show resolved Hide resolved
@gianfra-t gianfra-t force-pushed the 153-add-clippy-fix-to-pre-commit-script branch from 656c169 to 9a4dce9 Compare February 16, 2024 14:51
@gianfra-t gianfra-t force-pushed the 153-add-clippy-fix-to-pre-commit-script branch from 9a4dce9 to 22393b3 Compare February 16, 2024 15:59
@gianfra-t gianfra-t force-pushed the 153-add-clippy-fix-to-pre-commit-script branch from 346b384 to 325079f Compare February 16, 2024 17:18
@gianfra-t gianfra-t marked this pull request as ready for review February 19, 2024 14:07
@gianfra-t gianfra-t requested a review from a team February 19, 2024 14:07
Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

Can you please also update the README file

I assume that .maintain/add-prepush-hook.sh needs to be run manually once? Can you please also add this to the README?

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
.maintain/add-prepush-hook.sh Show resolved Hide resolved
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looking great, thanks 👍

@bogdanS98 bogdanS98 self-requested a review February 26, 2024 16:03
@gianfra-t gianfra-t merged commit b9623a7 into main Feb 28, 2024
1 of 2 checks passed
@gianfra-t gianfra-t deleted the 153-add-clippy-fix-to-pre-commit-script branch February 28, 2024 10:21
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.

Add clippy --fix to pre-commit script
4 participants