🧰👌 Use `pre-commit` instead of `tox -e style` to run all CQA tools

[s-weigand]

Currently, this project has 2 ways to run CQA tools:

• tox -e style
• pre-commit

To a new contributor, it might not be very clear how to run CQA tools.

Please don't take this the wrong way this is just my contributor experience it isn't meant as an attack.
I know first-hand that time for a FOSS maintainer is stretched VERY thin, sometimes you need to make hard calls like "tests pass, just linting fails so let's merge it, we will fix it later" and that typically no maintainer gets the praise that they deserve.
So thanks maintainers to not let a super nice project die and keeping it alive ❤️

My experience as a first-time contributor:

• I forked the repo and cloned my fork
• Installed the pre-commit git hooks (pre-commit install)
• And ran pre-commit run -a (as I usually do) to populate its cache with the tooling versions of this project
• Suddenly there were a lot of changes in a clean clone

$ git status       
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   .github/workflows/do-update-authors.yml
        modified:   CHANGELOG.md
        modified:   docs/source/authors.rst
        modified:   docs/source/license.rst
        modified:   src/docformatter/syntax.py
        modified:   src/docformatter/util.py
        modified:   tests/_data/string_files/do_format_code.toml
        modified:   tests/_data/string_files/do_format_docstrings.toml
        modified:   tests/_data/string_files/format_code.toml
        modified:   tests/formatter/test_do_format_docstring.py
        modified:   tests/test_configuration_functions.py
        modified:   tests/test_docformatter.py
        modified:   tests/test_encoding_functions.py
        modified:   tests/test_string_functions.py
        modified:   tests/test_syntax_functions.py
        modified:   tests/test_utility_functions.py

no changes added to commit (use "git add" and/or "git commit -a")

• So I git rest hard and uninstall the pre-commit git hooks (pre-commit uninstall)
• I do my changes and push to my fork to see that the do-lint workflow fails
• I have to look up how CQA tools are run (also took a moment since I didn't see a tox.ini) and run tox -e style locally
• tox -e style also fails in a clean clone because of unpinned versions and tooling updates
• Since ruff in .pre-commit-config.yaml was very outdated I pinned it to that version in the tox env (that version of the hook used a sys executable since it didn't define a pyproject.toml to pin the version) this should "just fix it" right?
• When I look at the sill failing CI I see that pycodestyle also has an error because linting on the default branch was broken since Aug 9, 2023
• Finally I can fix I wanted to fix

Advantages of running all CQA with pre-commit:

• Single source of truth
• Reproducibility due to version locking (... mostly since sometimes something down in the dependency tree just breaks things 🙈)
• Easier maintenance due to update functionality (pre-commit autoupdate)
• Clean commit history (no "Fix linting and formatting issues" commits)

Disadvantages of running all CQA with pre-commit:

• Additional PRs/commits due to updating of pinned versions of tools BUT on your own schedule (same as pinning versions in the tox config)

Alternative:

Remove the .pre-commit-config.yaml and use a tox.ini, so experienced python project contributors know how to contribute even without a contributing guide (at least I did see a guide prominently mentioned)

I know given my PR it might seem like "Given this issue why do you advertise for pre-commit?", but this is the first time that I experienced a problem that wouldn't have happened the same way with a fresh venv or tox.
While I have seen far more issues running tooling outside of pre-commit (e.g. see above).

So if maintainers are ok with fully changing to pre-commit as the CQA runner I would be happy to make a PR for this transition.

[webknjaz]

@s-weigand your issue title made me think tox.ini exists, however I don't see it anywhere, nor is it mentioned in the docs. I think we should start there. Having tox calling pre-commit in CI and dev env is pretty common. The problem seems to be that the CI is calling tox, but it's not actually set up in the first place...

[webknjaz]

Oh… So it's hidden away in pyproject.toml. In this case yes, I'd recommend sticking the pre-commit call into the tox env like so: https://github.com/ansible/awx-plugins/blob/ae95d1b/tox.ini#L195-L212.

[s-weigand]

Oh… So it's hidden away in pyproject.toml.

Yeah, that is what makes the project's tooling less obvious in general to people who have seen many projects and just guess tooling based on files.
Can't speak for anyone else but if I see a .pre-commit-config.yaml my lizard brain goes "Nice that is how all tooling is run." 😅
I know config file pollution in a repo is a thing which is why we tug things away in tox.ini, setup.cfg, and pyproject.toml and tools often support multiple files.
But using legacy_tox_ini where you basically dump a whole config file in a string and no non custom LSP or linter can help you out sounds horrible to maintain (no offense, just an opinion, I'm just too used to schema validation etc. telling me I messed up as soon as I mess up so no one else has to see it and tell me 😅)
I haven't used tox in a while (I just setup a venv with the lowest supported python version to no use too new syntax and check the rest on the CI) so I don't know what kind of tooling is out there to update pinned dependencies inside of a tox config.

But without version pinning IMHO running CQA becomes unpredictable.
E.g. you have a small change and while working on that there is an update to the formatter that wants to reformat half the code base
There are 3 options I see since CI will fail (or it won't if you get a cache hit):

• Create 2 PRs "update formatting" and "actual change/fix" I want to make
• Shove all in 1 PR (worst case for the reviewer/maintainer all in 1 commit)
• Since it is urgent and maintainers get pressured by angry users you merge it in even so the lining CI fails with a "We will fix it when we have time, it is just formatting" and because there is pretty much never enough time for FOSS maintenance (since most maintainers work for free in their spare time), you learn to live with that broken CI job and at some point it is more than just formatting

This is why I'm a huge pre-commit advocate since it was introduced to me 4y ago 😅

• Your tool versions are locked
• Cleaner git history (batteries included)
• There is an easy update command (more batteries included)
• One tool to run them all with support for multiple programming languages of tools (e.g. actionlint is written in go and I can just run it w/o installing go, maybe a more commonly used example is prettier)
• There also is pre-commit-ci (free for public repos) that gives you auto updates and by default (can be deactivated in your config) PR auto fixes OR if you like to not run the rest of the CI until the code is cleaned up you can use the GHA (also uses caching like tox-gh-actions) (since the CI would need to run again when the tooling is fine with it anyway)

[webknjaz]

Yeah, I've been using it pre-commit for over 8 years, if not more myself. Always wrapped with tox, and sometimes with pre-commit.ci in addition to that.

By the way, I saw Bernát tweeting the other day that he finally implemented a TOML config for tox. Need to check that out. tox-dev/tox#3353.

FWIW, here's my recent most integrated project example with "lock files" et al: https://github.com/ansible/awx-plugins/blob/ae95d1b/tox.ini.

[webknjaz]

As for discovering the tox envs, there's an old (tox 3) command tox -av that lists them and tox 4 has a nice one for printing them out — tox l.

[weibullguy]

@s-weigand Cut the PR if you want. I don't have a strong opinion one way or the other so long as all the tools get executed when they need to.

[finswimmer]

There are some inconsistency between the linting tools in the .pre-commit-config.yaml, those running via tox and those which are listed as dev-dependencies.

I can try to clean this up. @weibullguy Are there any strong opinions which tools should run and which one maybe can be replaced? (Lot's of the checks can be done by ruff now for example)

[weibullguy]

I can try to clean this up. @weibullguy Are there any strong opinions which tools should run and which one maybe can be replaced? (Lot's of the checks can be done by ruff now for example)

I have no problems with ruff replacing other tools.

[weibullguy]

PR #320 addresses this. Note that we need to be careful about running the style tools on the test directory because the test files contain purposely incorrect docstrings. I suspect all of those *.toml and *.py files that were modified were these incorrect docstrings (at least that's what happened when I ran the new tox pre-commit env the first time). I fixed all the errors in the other files that the pre-commit env uncovered and now, at least for me, running tox results in all green lights. If someone wants to offload more of the work onto ruff, we can cut a new PR to do that.