Replies: 2 comments 8 replies
-
|
Thanks for looking into this @lmilbaum. I agree with @JohnVillalovos I prefer how tox manages the environments over pre-commit's special venv handling, and it gives us a more native way of handling dependencies. I would also prefer to have that as the test & lint runner (or an equivalent environment manager). But I do use pre-commit as it sometimes still catches a few things before CI, and the main idea was to have that for contributors before opening PRs as it's a significant overhead on almost every PR review opened by new contributors and wastes CI resources. I guess I don't really have a good answer to this, if there was a way for pre-commit to use native python venvs from tox (e.g. the local hook approach), then maybe that'd be the best way. I should say |
Beta Was this translation helpful? Give feedback.
-
|
Those are valid points. Thank you for sharing your insights. WDYT? |
Beta Was this translation helpful? Give feedback.
-
|
To me, this still shifts a bit too much towards pre-commit (as we see with removing the requirements). If we could keep these things in pure python we can later make use of proper lockfiles to always ensure reproducible behavior. Is there any way to tell pre-commit to run its python-based hooks with the current python environment and installed dependencies, instead of tracking its own? That would be ideal. If it's too much work, I'd maybe remove the duplicates from the pre-commit config instead, and only run the commit message linter from there and any other hooks that do not exist in tox. Then we can instruct contributors to run tox before push, perhaps in a default PR template. Sorry for the detours here. |
Beta Was this translation helpful? Give feedback.
-
I see eye to eye with you on this. Reproduciable build (behavior) is a must have requirement in CI platform. My quetion would be, whether pinning the project revision (with a
Not sure about that. What if the project revision from the pre-commit config file produces conflict between what needs to be in the environment and what is already installed?
The effort of implementing this option is not significant. I woner though, what would be the user expriance. The user is a project contributor. How frictionless will it be to maintain linting/formatting checks split into two frameworks? Would switching to |
Beta Was this translation helpful? Give feedback.
-
Sorry, I meant something that we can manage fully using whatever dependency management we already use for our project, whether that is But I'm ok with just a simple solution. We can drop pre-commit and do everything in tox, IMO, as it'll be more lightweight. We just need to add some clear docs to run selected tox environments before pushing. The drawback will be a lot of people will have broken commits and will probably struggle locally to rebase interactively, but that's kind of already the case. |
Beta Was this translation helpful? Give feedback.
-
|
I guess I don't see this is a major issue having both. In my mind and in my usage, If we had to choose between both I would strongly vote for |
Beta Was this translation helpful? Give feedback.
-
|
Sounds like a good compromise! So @lmilbaum I just wouldnt duplicate the runs in CI then. So we'd keep tox in CI and have the hooks for local use. Keep in mind the pre-commit step in CI was initially made more to test that the pre-commit hooks actually work, not to run them on every PR. Maybe that was a source of confusion on why both were there. Hence these CI rules: python-gitlab/.github/workflows/pre_commit.yml Lines 9 to 22 in 4eca9b9 So we might need to take some of the standalone checks out or rework a bit. |
Beta Was this translation helpful? Give feedback.
-
|
The benefit to me of having And now I see @nejch said that 🙂 |
Beta Was this translation helpful? Give feedback.
-
|
Good discussion :-). Thanks for the engagement. Appologies for nit picking. As a DevOps practioner, this topic is in my core system. The list of requirements:
What we have now:
Hoping this summary covers all what we know. LMK if I missed something. With that summary, are we good to go with the approach suggested by @nejch |
Beta Was this translation helpful? Give feedback.
-
|
Sounds good to me! So de-duplicate, and I'm ok to add a tox env that does |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
The project is using two liniting frameworks: tox and pre-commit. Each has its own configuration files and a CI workflow.
That is a cause for some friction as the code is duplicated.
A followup from a discussion over #2320 with @JohnVillalovos
Beta Was this translation helpful? Give feedback.
All reactions