-
Notifications
You must be signed in to change notification settings - Fork 671
chore: remove pre-commit
as a default tox
environment
#2470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
84f85c9
to
db35bf0
Compare
pre-commit
as a default tox
environment
Wouldn't it make more sense to remove the other environments and to keep pre-commit? |
Not for me. I don't want to use pre-commit. |
John has some very strong opinions on pre-commit 😀 I also usually run a full We discussed this in #2321. I think we agreed we'd try to deduplicate this (see #2321 (reply in thread)), so I'd say the best way would be to have a tox environment that runs the missing checks only and that can be added to the default. But I'm ok to do that as a follow-up and merge this |
For users who use `tox` having `pre-commit` as part of the default environment list is redundant as it will run the same tests again that are being run in other environments. For example: black, flake8, pylint, and more.
db35bf0
to
2d98766
Compare
I got that @JohnVillalovos dislikes pre-commit. :-) I can't bit that with my engineering arguments. |
There's another issue with this I just realized. pre-commit keeps re-initializing on almost every run because there's always an outdated hook, which can take a while, even if the tox dependencies are otherwise up-to-date. Takes away from quick "shift left" hooks IMO, maybe let's remove it for now and find another way later. |
For users who use
tox
havingpre-commit
as part of the defaultenvironment list is redundant as it will run the same tests again that
are being run in other environments. For example: black, flake8,
pylint, and more.