lint: add pre-commit config #582
Conversation
This allows re-using the same config in tox and other tools, e.g. pre-commit. Signed-off-by: Lukas Puehringer <[email protected]>
Add basic pre-commit config file, with hooks for black and isort, plus two convenient builtin checkers. Also adds pre-commit to dev requirements and removes the config file name from .gitignore Caveat: hooks in config file must be manually initialzed and updated, e.g. with the following commands: ``` pre-commit install pre-commit autoupdate ``` Signed-off-by: Lukas Puehringer <[email protected]>
requirements-dev.txt
Outdated
| @@ -2,6 +2,7 @@ | |||
| # requirements for local testing with tox, and also for the running test suite | |||
| # or individual tests manually | |||
| tox | |||
| pre-commit | |||
There was a problem hiding this comment.
I have no strong feelings about including this here or not. Given that devs must enable hooks manually, we could also ask them to install pre-commit themselves.
There was a problem hiding this comment.
Adding just pre-commit is not enough I assume. You would also need to add black and isort, right? (or maybe I don't understand what pre-commit handles).
In any case i'd lean on not adding it here.
There was a problem hiding this comment.
Adding just pre-commit is not enough I assume. You would also need to add black and isort, right?
No, pre-commit is a package manager and will install black and isort pre-commit plugins from GitHub, where they are managed by the upstream authors themselves via .pre-commit-hooks.yaml.
In any case i'd lean on not adding it here.
Works for me. I'll remove it.
|
FYI: In python-tuf we decided to not include the pre-commit config file, because of the caveat mentioned above: theupdateframework/python-tuf#1314 (review) In hindsight, I think, the convenience of including the file outweighs the burden of additional maintenance. |
| repos: | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v4.4.0 | ||
| hooks: | ||
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer | ||
|
|
||
| - repo: https://github.com/psf/black | ||
| rev: 23.3.0 | ||
| hooks: | ||
| - id: black | ||
| args: ["."] | ||
|
|
||
| - repo: https://github.com/pycqa/isort | ||
| rev: 5.12.0 | ||
| hooks: | ||
| - id: isort | ||
| args: ["."] |
There was a problem hiding this comment.
I'm not forced to worry about this so it's not a big deal... but:
- is it a good idea to have two slightly different sets of linting passes (
tox -e lintand this pre-commit list)? - do we understand the dependency chains here? I assume black and isort hooks depend on already installed tools (or does
pre-commit installmagically install them from somewhere?)... but what about the "builtin" hooks? Do they use code embedded in the pre-commit package? - are we in trouble when we inevitably forget to update these versions? I suppose not?
There was a problem hiding this comment.
is it a good idea to have two slightly different sets of linting passes (tox -e lint and this pre-commit list)?
I don't see a big problem there. Config is the same for both. Differences would mainly come from different versions of black && isort. But tox -e lint on CI is the ultimate authority.
do we understand the dependency chains here? I assume black and isort hooks depend on already installed tools (or does pre-commit install magically install them from somewhere?)... but what about the "builtin" hooks? Do they use code embedded in the pre-commit package?
I admit, I don't exactly know where pre-commit installs the dependencies, but I suppose in some virtual environment. It claims that "python hooks work without any system-level dependencies".
are we in trouble when we inevitably forget to update these versions? I suppose not?
I don't think, we are. But yeah, it would be nice to automate updates. There are some 3rd-party solutions to do this, and a feature request for Dependabot.
requirements-dev.txt
Outdated
| @@ -2,6 +2,7 @@ | |||
| # requirements for local testing with tox, and also for the running test suite | |||
| # or individual tests manually | |||
| tox | |||
| pre-commit | |||
There was a problem hiding this comment.
Adding just pre-commit is not enough I assume. You would also need to add black and isort, right? (or maybe I don't understand what pre-commit handles).
In any case i'd lean on not adding it here.
Good question. I haven't used isort && black with vanilla git hooks. Main reason for me was the easy setup and use. Plus it seemed popular enough, especially the enabled plugins, to be relatively trustworthy. |
pre-commit is optional for devs, we should not make everyone install it. Signed-off-by: Lukas Puehringer <[email protected]>
Add basic pre-commit config file, with hooks for black and isort, plus two convenient builtin checkers.
Related changes in this PR
Caveat
hooks in config file must be manually initialized and updated: