Update pre commit hooks #1774
Update pre commit hooks #1774
Conversation
| hooks: | ||
| - id: black | ||
| language_version: python3 | ||
| rev: 21.7b0 |
There was a problem hiding this comment.
is it possible to make this work without setting the value rev?
It would add technical debt..
There was a problem hiding this comment.
It's set by pre-commit autoupdate.
There was a problem hiding this comment.
Requiring hooks to have pinned versions is a core design decision of pre-commit, since it ensures that all developers and CIs get the same consistent version of hooks for a given project repository commit, and thus hooks don't arbitrarily fail or make different changes based on whatever version happens to be installed on the current platform. There's also a GH action that, like dependabot, automatically submits PRs at a reasonable frequency to upgrade out of date hooks.
.flake8
Outdated
| per-file-ignores = | ||
| praw/models/__init__.py:F401 | ||
| praw/models/listing/mixins/__init__.py:F401 | ||
| praw/models/reddit/mixins/__init__.py:F401 |
There was a problem hiding this comment.
careful about newline at EOF
There was a problem hiding this comment.
FYI, there's a built-in hook to fix this problem automatically, end-of-file-fixer. You might be interested in some of the other out of the box pre-commit-hooks as well; they are very cheap to run and can fix many common problems automatically.
| ignore = E203 E501 W503 W504 | ||
| per-file-ignores = | ||
| praw/models/__init__.py:F401 |
There was a problem hiding this comment.
I would even go as far as to do
praw/*/__init__.py
There was a problem hiding this comment.
I think it's fine as is.
| @@ -1,9 +1,2 @@ | |||
| [aliases] | |||
There was a problem hiding this comment.
Why is the purpose of the setup.cfg here if you already have tox.ini giving instructions on the test
There was a problem hiding this comment.
This is settings for setuptools.
There was a problem hiding this comment.
FYI, this setting and setup.py test is deprecated and will trigger a warning in new versions of setuptools, and will eventually be removed.
|
Great to see this @LilSpazJoekp , and sorry I wasn't able to help, too busy working on Sub Manager unfortunately (mostly our own extensive pre-commit config). I'll give it a look-over as well in case I spot anything. |
There was a problem hiding this comment.
Just to note, given that all the now-duplicate dep installation and lint runner commands lint-multi-os job in your CI workflow (aside from the Sphinx build) is now also covered here, I would suggest replacing all that redundancy with the pre-commit Github action which will install and run them through pre-commit, and tweak your caching as needed. This ensures the same checks are run locally as in the CI (right now some of the invocations are different, because they were changed here but not there, and the versions may drift out of sync), keeps things DRY and avoids doubling the maintenance burden, with corresponding issues if things get out of sync.
Similarly, now that you've done this, I suggest not further duplicating the invocations and versions again in pre_push.py and the setup.py lint extra respectively, and instead replacing all that with a single call to pre-commit, i.e. pre-commit run or pre-commit run --all-files. In all, this triples the maintenance burden of keeping versions up to date and tweaking invocations (particularly since pre-commit handles the former and much of the latter for you), again ensures consistent behavior between the script and the hooks, and brings the benefits of pre-commit's automated, platform-independent installation, updating, output reporting and more.
|
@CAM-Gerlach I like your suggestions. Would you mind opening a PR to this branch for what you suggest? |
LilSpazJoekp
force-pushed
the
update_pre_commit_hooks
branch
2 times, most recently
from
7a7f415 to
0d2249f
Compare
|
Sure, finishing up something now but I'll try to get to it later this evening (CDT), or if not then tomorrow. If that holds up merging this too long, I'm happy to make another PR. Thanks! |
LilSpazJoekp
force-pushed
the
update_pre_commit_hooks
branch
from
0d2249f to
55de585
Compare
|
@CAM-Gerlach I'm going to go ahead and merge this. If you have suggested changes feel free to open a PR for it. |
|
@LilSpazJoekp Hey, really sorry for the long delay; it was on my task list but with priority NASA projects, other open-source stuff that came up and an illness, I never quite got to it. I'll submit a PR soon once I get the chance, thanks. |
This just updates the pre-commit hooks to do the same thing
pre_push.pydoes. It also moves and consolidates tool configurations.