FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore: remove `pre-commit` as a default `tox` environment by JohnVillalovos · Pull Request #2470 · python-gitlab/python-gitlab · GitHub

Repository navigation

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

Merged
nejch merged 2 commits into
mainfrom
jlvillal/pre_commit_check
Feb 5, 2023
Merged

nejch merged 2 commits into
mainfrom
jlvillal/pre_commit_check

Conversation

JohnVillalovos commented Feb 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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 force-pushed the jlvillal/pre_commit_check branch from 84f85c9 to db35bf0 Compare February 3, 2023 01:33
JohnVillalovos changed the title WIP: testing... chore: remove pre-commit as a default tox environment Feb 3, 2023
JohnVillalovos requested a review from nejch February 3, 2023 01:33

lmilbaum commented Feb 3, 2023

Copy link
Copy Markdown

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
Copy Markdown
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 commented Feb 3, 2023

Copy link
Copy Markdown
Member

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

lmilbaum commented Feb 4, 2023

Copy link
Copy Markdown

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 commented Feb 5, 2023

Copy link
Copy Markdown
Member

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 enabled auto-merge (squash) February 5, 2023 21:19
nejch merged commit fde2495 into main Feb 5, 2023
nejch deleted the jlvillal/pre_commit_check branch February 5, 2023 21:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL