Skip to content

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