Use pre-commit#11945
Conversation
| # - repo: https://github.com/asottile/pyupgrade | ||
| # rev: v2.21.0 | ||
| # hooks: | ||
| # - id: pyupgrade |
There was a problem hiding this comment.
2 additional checks we could use:
- we have an isort config, should we run it (I thought we ran it once in the past but actually it seems not)
- pyupgrade was used to convert to fstrings in Convert format and % strings to f-strings #8958, it can also provide various little improvements (https://github.com/asottile/pyupgrade) using syntax from the recent versions of Python.
There was a problem hiding this comment.
Other good built-in ones (v3.4.0>) are:
check-added-large-files
check-case-conflict
check-merge-conflict
check-symlinks
check-toml
check-yaml
debug-statements
destroyed-symlinks
detect-private-key
end-of-file-fixer
fix-encoding-pragma
requirements-txt-fixer
trailing-whitespace
There was a problem hiding this comment.
isort config, should we run it
👍
There was a problem hiding this comment.
And https://github.com/pre-commit/pygrep-hooks is great for rst checks.
- rst-backticks
- rst-directive-colons
- rst-inline-touching-normal
I also use it for checking that I give the right error message in noqa e.g. noqa: F403
- python-check-blanket-noqa
There was a problem hiding this comment.
Other good built-in ones (v3.4.0>) are:
I already added most of them:
- id: check-added-large-files
- id: check-case-conflict
- id: check-yaml
- id: debug-statements
- id: end-of-file-fixer
- id: trailing-whitespace
There was a problem hiding this comment.
Looks like they are working! The CI has found oodles of corrections in the code.
|
@pllim - No, it just runs on all changelog fragments, and cannot check that a given PR actually has a fragment. And it couldn't be skipped with some label. |
|
For the changelog check (@bsipocz @eteq), here is what it gives: |
|
minor idea: maybe this should also check whether the changelog is too wordy, e.g. notify the reviewer to double-check the entry if it's longer than ~5 lines? |
pllim
left a comment
There was a problem hiding this comment.
Instead of adding all sorts of checks in this PR, perhaps we should go with what is already here and defer the rest for follow-up PRs.
Release managers should carefully review the change log check, as it will affect them the most at release time.
I guess I could make it fail if a fragment has more than 5 lines, but then with the way it works it would always fail until you fix that file. So it would prevent having exceptions unless adding the file to the exceptions in pre-commit's config, which would not be practical. |
what about a new special label? :) |
|
pre-commit does not access github labels and does not know anything about the current PR. It just runs a number of checks on a selection of files. So for the changelog fragments I just tell it to check that files in |
|
I checked out this branch and ran |
|
Rerunning the CI to see if the code style check job error is real... @saimn it appears to be real. |
|
Oops, in the initial version I had pre-commit calling tox for flake8, and then I updated tox to call pre-commit 😁 |
pllim
left a comment
There was a problem hiding this comment.
This is a good improvement to help maintainers. Let's get it in! We can always tweak it later, as needed.
I was looking for a way to run checks for the changelog fragment filenames and pre-commit seems a good option. Actually tox already has such a check so I adapted theirs. With this pre-commit will check that the fragments have one of the correct categories (bugfix|feature|api|other), and that files with the "other" category are put only in the root directory (following #11856 (comment)).
I also added some basic hooks for trailing whitespace and end of lines (@Cadair proposed a similar config in #9890)
About flake8: I added a hook to run flake8 via
tox -e codestyle. In #9890 @Cadair did it another way by running pre-commit from tox. After playing with this, I think we should remove flake8 / codestyle from the tox config and do it directly with pre-commit, there is no real benefit in having the possibility to run flake8 with both. And since pre-commit is about running code style checks, flake8 should be part it.If we go with this, we will need to run it once to fix the existing files, I can do that in this PR later (after 4.3 is out and fragments have been deleted in main).