chore: bootstrap pre-commit config#99
Conversation
Adds meta, pre-commit-hooks (trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-merge-conflict), and local uv-run hooks for ruff, ruff-format, and codespell. Pins ruff>=0.11,<0.12 and adds codespell>=2.4,<2.5 to the lint dependency group; regenerates uv.lock.
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
I'm not a fan of pre-commit hooks in general, and don't run them myself.
I'm fine with including them in our repositories if they don't introduce a second source of truth for checks.
It would also be nice to continue to keep our external dependencies to a minimum, and ensure that they're managed the same way we manage our other dependencies (dependabot, security alerts, etc).
| - id: check-hooks-apply | ||
| - id: check-useless-excludes | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v5.0.0 |
There was a problem hiding this comment.
Can pre-commit pin by hash? Does dependabot understand and bump these dependencies?
| - id: ruff | ||
| name: ruff | ||
| entry: uv run ruff check --fix | ||
| language: system | ||
| types_or: [python, pyi] | ||
| require_serial: true | ||
| - id: ruff-format | ||
| name: ruff-format | ||
| entry: uv run ruff format | ||
| language: system | ||
| types_or: [python, pyi] | ||
| require_serial: true | ||
| - id: codespell | ||
| name: codespell | ||
| entry: uv run codespell | ||
| language: system | ||
| types: [text] | ||
| require_serial: true |
There was a problem hiding this comment.
I think we've talked about this before, but I still don't really understand why it wouldn't be better to call tox from pre-commit, rather than introducing a second source of truth for linters and formatters. I'd suggest calling tox -e format and adding a fast-lint environment to capture the fast linters we'd run on pre-commit.
There was a problem hiding this comment.
For one thing, pre-commit (or prek) is much smarter about files. It won't do checks that aren't relevant for the file type, it can do checks concurrently, and it ensures it's only running on the files that are staged for the commit (the later might still work calling another runner). It also adds an extra layer for what seems like little benefit when Make/tox is just running exactly the same uv command with config and version coming from pyproject.toml. Also, while we're still using tox, it avoids the whole mess that our tox-uv usage gives.
There was a problem hiding this comment.
For one thing, pre-commit (or prek) is much smarter about files. It won't do checks that aren't relevant for the file type
This is a great feature indeed, but orthogonal to the source of truth. You can have types_or: [python, pyi] gate running tox -e format, for example.
it can do checks concurrently
This also seems like it would be independent of where the source of truth for the check is. I notice that the three checks defined here all use require_serial: true, which I assume means they're not running concurrently.
and it ensures it's only running on the files that are staged for the commit (the later might still work calling another runner).
This also seems kind of orthogonal. If pre-commit can smartly pass the right files to uv run ruff format, then surely it can pass them to our task runner command too. For example, if it passes them positionally, then couldn't the command be tox -e format --?
It also adds an extra layer for what seems like little benefit when Make/tox is just running exactly the same uv command with config and version coming from pyproject.toml.
They're the same right now, but any extra flags or commands then need to be duplicated in two places. The benefit is having the commands we use for (e.g.) linting be defined in just one place. For example, we call tox -e lint in CI, rather than uv run ruff check.
Also, while we're still using tox, it avoids the whole mess that our tox-uv usage gives.
What mess do we run into here?
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer | ||
| - id: check-yaml | ||
| - id: check-toml |
There was a problem hiding this comment.
These seem like they could be useful if they could be enabled generally, for example if ruff format can be configured to handle the whitespace fixes, or if we could include a YAML or TOML linter in our tox -e lint.
There was a problem hiding this comment.
I'd rather not add extra dependencies, when this one tool we already use provides the functionality. ruff is focused (at least for now) on Python files; it's not going to handle the others.
We could write custom code for it - I would guess < 100 lines if we're ignoring edge cases. One more thing to maintain in exchange for using fewer dependencies in pre-commit and being able to more easily use it outside.
I could add to tox -e lint pre-commit run --all-files --show-diff-on-failure with a SKIP to avoid checks tox is already running. Then everyone would be running pre-commit (including CI), which is more consistent but pushes everyone to use the tool (installing the hooks would still be optional). Would you prefer that?
There was a problem hiding this comment.
I'd rather not add extra dependencies, when this one tool we already use provides the functionality. ruff is focused (at least for now) on Python files; it's not going to handle the others.
It seems like we're already adding extra dependencies in our pre-commit for exactly these checks.
I could add to
tox -e lintpre-commit run --all-files --show-diff-on-failurewith aSKIPto avoid checks tox is already running.
I'm not opposed to running checks that there's no good non-pre-commit alternative to from tox. pre-commit seems to be available as a Python package, so no extra steps would be needed for users to run the checks this way. It bothers me a little that this makes the checks only run in a git repository, but we are in a git repository, so that's OK. We'd need to be careful about cycles if we have some checks defined in pre-commit (called by tox) and others defined in tox (potentially called by pre-commit).
I'm a huge fan, and even more so with agents writing and committing the code. I had thought we had previously decided pre-commit config was ok for Charm Tech, particularly since it's opt in so can be ignored (other than one additional top-level dot file) for anyone that doesn't want to use it. If that's wrong or you would prefer we discuss it again, I'm fine bringing it to daily before rolling it out as part of the roadmap item. |
Yes, that's why the pre-commit across all our repos (the ones with them already, plus the standardisation done for the roadmap) uses "local" everywhere so the versioning is from pyproject.toml, is managed by dependabot, and so on. |
I was thinking about this bit: |
I'm definitely not opposed to having pre-commit config at all, it just feels wrong when it defines a parallel set of linting commands. I recall for a while in Ops our pre-commit and tox diverged a bit. Could be worth a discussion. |
Yes, which we fixed by moving to local and having all the config and versions in pyproject.toml instead of duplicated. |
3 participants
Adds meta, pre-commit-hooks (trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-merge-conflict), and local uv-run hooks for ruff, ruff-format, and codespell.