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 pre-commit hook for formatting, linting, and sorting #172

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from

Conversation

diegomrsantos
Copy link

@diegomrsantos diegomrsantos commented Mar 5, 2025

Proposed Changes

Create a pre-commit file in .githooks. You only need to run git config core.hooksPath .githooks and the old days of CI failures are over.

@diegomrsantos diegomrsantos self-assigned this Mar 5, 2025
@diegomrsantos diegomrsantos added chore ready-for-review This PR is ready to be reviewed labels Mar 5, 2025
@dknopik
Copy link
Member

dknopik commented Mar 6, 2025

I think invoking the appropriate commands in the Makefile would be more appropriate, in order to use the flags specified there. i.e. make lint

@diegomrsantos
Copy link
Author

I think invoking the appropriate commands in the Makefile would be more appropriate, in order to use the flags specified there. i.e. make lint

I agree. Are they make cargo-fmt, make lint, and make sort?

@diegomrsantos
Copy link
Author

@dknopik should we get it merged like this?

@dknopik
Copy link
Member

dknopik commented Mar 10, 2025

@diegomrsantos how about the switch to invoking the make targets?

@diegomrsantos
Copy link
Author

@diegomrsantos how about the switch to invoking the make targets?

The make targets have an error that causes the script to fail. Additionally, the fmt and lint ones only check and don't apply fixes.

@dknopik
Copy link
Member

dknopik commented Mar 10, 2025

The make targets work fine on my end, and are also what's used in CI. But I will likely not use this anyway 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore ready-for-review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants