search

PyCQA / docformatter

Formats docstrings to follow PEP 257
https://pypi.python.org/pypi/docformatter
MIT License
549 stars 68 forks source link

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

Open s-weigand opened 1 month ago

s-weigand commented 1 month ago

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

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:

Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." 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](https://github.com/s-weigand/docformatter/actions/runs/11195813262/job/31123914169)
- 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 ](https://github.com/astral-sh/ruff-pre-commit/blob/v0.0.269/.pre-commit-hooks.yaml) since it didn't define a [pyproject.toml](https://github.com/astral-sh/ruff-pre-commit/blob/main/pyproject.toml) to pin the version) this should "just fix it" right?
- When I look at the [sill failing CI](https://github.com/s-weigand/docformatter/actions/runs/11195882747/job/31124075968) I see that `pycodestyle` also has an error because linting on the default branch was [broken since Aug 9, 2023](https://github.com/PyCQA/docformatter/commit/5948f6dca999e5b8dae39349e08f20f65935acf6)
- Finally I can [fix I wanted to fix](https://github.com/PyCQA/docformatter/pull/287)

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](https://github.com/PyCQA/docformatter/pull/287) 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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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):

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

webknjaz commented 1 month ago

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. https://github.com/tox-dev/tox/pull/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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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)