NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 2 commits intomainfromjlvillal/pre_commit_check

Feb 5, 2023

Merged

chore: remove`pre-commit` as a default`tox` environment#2470

nejch merged 2 commits intomainfromjlvillal/pre_commit_check

Feb 5, 2023

Conversation

Copy link

Member

JohnVillalovos commentedFeb 3, 2023•
edited
Loading

For users who usetox havingpre-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.

JohnVillalovos force-pushed thejlvillal/pre_commit_check branch from84f85c9 todb35bf0Compare

February 3, 2023 01:33

JohnVillalovos changed the title~~WIP: testing...~~chore: removepre-commit as a defaulttox environment

Feb 3, 2023

JohnVillalovos requested a review fromnejch

February 3, 2023 01:33

Copy link

lmilbaum commentedFeb 3, 2023

Wouldn't it make more sense to remove the other environments and to keep pre-commit?
pre-commit performs validations which are not covered by the granular ones.

Copy link

MemberAuthor

JohnVillalovos commentedFeb 3, 2023

Wouldn't it make more sense to remove the other environments and to keep pre-commit?
pre-commit performs validations which are not covered by the granular ones.

Not for me. I don't want to use pre-commit.

Copy link

Member

nejch commentedFeb 3, 2023

Wouldn't it make more sense to remove the other environments and to keep pre-commit?
pre-commit performs validations which are not covered by the granular ones.
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 fulltox locally before pushing a new PR, and also prefer the native python dependency management of that over pre-commit, at least for tests etc. I also get the full duplicate suite run now which takes a while (it used to be ~15-20 seconds IIRC).

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

chore: removepre-commit as a defaulttox environment

2d98766

For users who use `tox` having `pre-commit` as part of the defaultenvironment list is redundant as it will run the same tests again thatare being run in other environments. For example: black, flake8,pylint, and more.

JohnVillalovos force-pushed thejlvillal/pre_commit_check branch fromdb35bf0 to2d98766Compare

February 4, 2023 01:14

Copy link

lmilbaum commentedFeb 4, 2023

Wouldn't it make more sense to remove the other environments and to keep pre-commit?
pre-commit performs validations which are not covered by the granular ones.
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 fulltox locally before pushing a new PR, and also prefer the native python dependency management of that over pre-commit, at least for tests etc. I also get the full duplicate suite run now which takes a while (it used to be ~15-20 seconds IIRC).
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

I got that@JohnVillalovos dislikes pre-commit. :-) I can't bit that with my engineering arguments.

Copy link

Member

nejch commentedFeb 5, 2023

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.