Skip to content

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

Merged
merged 2 commits into from
Feb 5, 2023

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Feb 3, 2023

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.

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/pre_commit_check branch from 84f85c9 to db35bf0 Compare February 3, 2023 01:33
@JohnVillalovos JohnVillalovos changed the title WIP: testing... chore: remove pre-commit as a default tox environment Feb 3, 2023
@JohnVillalovos JohnVillalovos requested a review from nejch February 3, 2023 01:33
@lmilbaum
Copy link

lmilbaum commented Feb 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.

@JohnVillalovos
Copy link
Member Author

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.

@nejch
Copy link
Member

nejch commented Feb 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 full tox 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

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.
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/pre_commit_check branch from db35bf0 to 2d98766 Compare February 4, 2023 01:14
@lmilbaum
Copy link

lmilbaum commented Feb 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 full tox 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.

@nejch
Copy link
Member

nejch commented Feb 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.

@nejch nejch enabled auto-merge (squash) February 5, 2023 21:19
@nejch nejch merged commit fde2495 into main Feb 5, 2023
@nejch nejch deleted the jlvillal/pre_commit_check branch February 5, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants