Enable Black and other pre-commit hooks#744
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
Looks fairly straightforward - although there's already a merge conflict because of the formatting changes from #743 :-)
A couple of questions/clarifications inline about the specifics on how will play out in practice.
| with: | ||
| python-version: "3.9" | ||
| - name: Lint with Pre-commit | ||
| uses: pre-commit/action@v2.0.3 |
There was a problem hiding this comment.
Does this modify the code, or just run flake/black/etc checks and report an error if they haven't been run? If it does modify - who is the author of the commit?
There was a problem hiding this comment.
Exits with an error if pre-commit would need to modify a file.
There was a problem hiding this comment.
Ok; makes sense. However, we may need to restructure the CI workflow to accomodate. At present, the
"beefore" CI step is being run as a matrix, so there are 3 parallel tasks; this is running pre-commit on each of them, so a black violation is reported 3 times (see this test run on #746 for an example); and it looks like the failure is due to "town crier", rather than the underlying flake8 issue.
There was a problem hiding this comment.
Done, moved pre-commit to the first action run, then beefore follows. Another option is, we could make use of https://pre-commit.ci. An advantage is it runs faster and automatically creates PRs with the pre-commit hooks are out of date. This could be a future thing we look at, though.
Merge conflict fixed, I rebased it! |
freakboy3742
left a comment
There was a problem hiding this comment.
I've had a chance to play around with pre-commit; I'm not in love with the onboarding experience (i.e., there's no way to signal to developers that they're missing something in their config) - but I'm not sure if there's anything we can do about that.
I've flagged two specific places that need some more attention in the specific configuration of pre-commit.
| with: | ||
| python-version: "3.9" | ||
| - name: Lint with Pre-commit | ||
| uses: pre-commit/action@v2.0.3 |
There was a problem hiding this comment.
Ok; makes sense. However, we may need to restructure the CI workflow to accomodate. At present, the
"beefore" CI step is being run as a matrix, so there are 3 parallel tasks; this is running pre-commit on each of them, so a black violation is reported 3 times (see this test run on #746 for an example); and it looks like the failure is due to "town crier", rather than the underlying flake8 issue.
| rev: v4.2.0 | ||
| hooks: | ||
| - id: check-toml | ||
| - id: check-yaml |
There was a problem hiding this comment.
It would be worth auditing the full list of available hooks to see if there's anything else useful there. Other projects (like pyscript or pip) may be a good reference here.
There was a problem hiding this comment.
Added a few more for trailing whitespace, newline for end-of-file, etc. 👍
There was a problem hiding this comment.
One additional one that would be good would be to add mypy as a pre-commit hook, even if we aren't planning on or planning on adding incremental type hints. It's analysis found errors, and we could resolve these and make it part of a future PR.
|
Updates made! Hopefully I don't have to rebase this one again 🙏 😄 |
Codecov Report
|
freakboy3742
left a comment
There was a problem hiding this comment.
I've made a couple of documentation tweaks and similar cosmetic cleanups, but otherwise, this looks good to go. There's a couple more pre-commit checks that might be worth integrating - but given the overhead associated with maintaining this patch (and the impact it has on other patches), I think it's worth landing as-is, and dealing with those additions as follow-on work.
Thanks for the prod (and the legwork) to make this happen!
| .. code-block:: doscon | ||
|
|
||
| C:\...>pip install tox | ||
| C:\...>pip install -r requirements.test.txt |
There was a problem hiding this comment.
requirements.test.txt is the set of requirements installed by tox; it would be better for us to include tox in the dev requirements.
|
Thanks @freakboy3742 ! 🙌 |
Enables pre-commit hooks for:
Adds a .git-blame-ignore-revs file for keeping git blame history. Removes flake8 from tox since it is part of the pre-commit hook. Updates the CI to include the pre-commit hooks as part of the beefore step.
PR Checklist: