sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.46k stars 2.1k forks source link

tox targets for non-tests #11599

Closed AA-Turner closed 3 months ago

AA-Turner commented 1 year ago

@AA-Turner Could I ask what was the rationale for this change? How are contributors expected to run e.g. linters on their changes without the tox targets?

Originally posted by @stephenfin in https://github.com/sphinx-doc/sphinx/commit/ff852bc7c31f48e66100e4647749fc199d92ca79#commitcomment-124548969

AA-Turner commented 1 year ago

Could I ask what was the rationale for this change?

I don't use tox, and I feared the tox commands becoming out of date.

How are contributors expected to run e.g. linters on their changes without the tox targets?

I'd expect people to just run the linters, e.g.:

> ruff check .
> mypy sphinx
> flake8 .
> isort .

This is in the documentation but perhaps could be better placed? https://www.sphinx-doc.org/en/master/internals/contributing.html#coding-style

A

picnixz commented 1 year ago

This is in the documentation but perhaps could be better placed?

How about creating a make target? like make lint (that's what I usually have in my personal projects).

(It seems that there are already two of such targets, namely style-check for flake8 and type-check for mypy).

stephenfin commented 1 year ago

Thanks for opening this :+1:

Could I ask what was the rationale for this change?

I don't use tox, and I feared the tox commands becoming out of date.

That couldn't ever happen so long as the CI was using the tox environments, right? The removal of tox from CI introduced the possibility for things to get out of sync.

How are contributors expected to run e.g. linters on their changes without the tox targets?

I'd expect people to just run the linters, e.g.:

> ruff check .
> mypy sphinx
> flake8 .
> isort .

This is in the documentation but perhaps could be better placed? sphinx-doc.org/en/master/internals/contributing.html#coding-style

The problem with this approach is that it requires manual configuration of an environment - likely a virtualenv - plus invocation of multiple commands in order to run against all linters. The use of a make target would avoid the latter but still require the former. This is the principal raison d'être for tools like tox and nox.

It would probably be good to understand how you're working and what issues you had with using tox in CI so that we can find a solution that works for both? My guess is that you have these installed in a local virtualenv and you're relying on IDE/editor integration (VSCode) to do your linting? As a periodic contributor to Sphinx, I personally don't want to have to read through documentation to figure out what linters I need to run to satisfy the CI. This is unlikely to be a one-and-done kind of thing nor something I would commit to muscle memory given the many months that can go by without any contribution to Sphinx. Instead, I would like linter configuration to be done via a "standard" mechanism, which for the Python ecosystem is a tool like tox. I suspect this would likely be the case for most other infrequent/drive-by contributors.

stephenfin commented 1 year ago

I've proposed #11602 as a potential solution. Another option is to use pre-commit but that's additional work that still wouldn't address the lack of a tox environment.

picnixz commented 1 year ago

Concerning pre-commit, we have a discussion here #11205.

stephenfin commented 1 year ago

Concerning pre-commit, we have a discussion here #11205.

Good to hear there's already interest in this. I've pushed up my alternate solution using pre-commit to #11603.