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

Use same ruff command on CI and in pre-commit #617

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

vjousse
Copy link
Collaborator

@vjousse vjousse commented Jun 5, 2024

🔧 Problem

Currently we have different linting rules for ruff in the CI and in the pre-commit hook (we use --extended-select I in the CI/package.json and we don't in the pre-commit, error on my side).

We use a dedicated pre-commit hook to fix/lint the python code when committing, but on the CI we use our usual npm lint:all script to check that everything is ok.

If we modify our linting in package.json, it will not be reflected on the pre-commit hook and vice-versa.

🍰 Solution

Use npm fix commands in the pre-commit hook to be consistent with the CI.

🚨 Points to watch / comments

I've added the --force-exclude to the ruff params so that it excludes files that are mentioned in pyproject.toml (it's not the default behavior when a filename is specified as a parameter and that's what pre-commit does)

The format and check commands for ruff are a little bit weird. format require a --check flag to not write changes to files, and check requires the opposite (--fix) to write changes to files. At some point they will be unified (see astral-sh/ruff#8232) but it's not the case yet.

🏝️ How to test

npm lint:all and npm fix:all should work as usual.

@vjousse vjousse requested review from n1k0, magopian and ccomb June 5, 2024 14:49
@vjousse vjousse self-assigned this Jun 5, 2024
@vjousse vjousse merged commit 178f84c into master Jun 12, 2024
5 checks passed
@vjousse vjousse deleted the fix/consistent-ruff-linting branch June 12, 2024 07:35
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.

2 participants