Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove linters from requirements-tests.txt #12725

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

AlexWaygood
Copy link
Member

These days, we do all our linting in CI via pre-commit; linters are only listed in requirements-tests.txt for local convenience, as pre-commit uses an isolated virtual environment for each linting tool it executes. But running pre-commit run -a black isn't really much more to type than black, and installing all these linters into the same environment is starting to cause issues. (See: the CI failures in #12717.) Flake8-pyi has dropped support for Python 3.8 in its latest version, and other linters are likely to soon follow, but per the consensus in #12716 we probably won't do so until the new year. If we're happy to do our linting via pre-commit locally as well as doing it via pre-commit in CI, that makes the situation caused by some tools dropping support for Python 3.8 quite a lot less problematic.

The only linter that I think we do need to keep in requirements-tests.txt is Ruff, because we run it as a subprocess from create_baseline_stubs.py, and we run it in that script with slightly different arguments to the ones we use in our pre-commit config file.

If we don't want to go this route, then we can just use environment markers in the requirements-tests.txt file to specify that the linting dependencies only need to be installed on Python 3.9+. We did this for a while after flake8 and flake8-pyi dropped support for Python 3.7 (until 487e331), and it worked fine. The main disadvantages of that approach are (1) that it feels like a bit of a hack, and (2) we'll probably run into the same issue again when Python 3.9 goes EOL in around a year's time. This approach solves it for the long term.

@AlexWaygood AlexWaygood added the project: infrastructure typeshed build, test, documentation, or distribution related label Oct 2, 2024
AlexWaygood added a commit that referenced this pull request Oct 2, 2024
Fixes a CI failure seen in  #12725
@AlexWaygood
Copy link
Member Author

I'll leave this open for a bit in case this severely breaks the workflow of any other maintainers :-)

@Avasam
Copy link
Collaborator

Avasam commented Oct 2, 2024

I believe uv supports https://peps.python.org/pep-0723/ , it might help go in that direction by supporting uv run tests/runtests.py, uv run scripts/create_baseline_stubs.py, etc. w/o pre-installing requirements. Although we might end up with duplicate specifications? Unless renovate adds support for it, then duplication wouldn't matter. idk, something to consider.
(message edited to consider renovate)

@AlexWaygood
Copy link
Member Author

I believe uv supports peps.python.org/pep-0723 , it might help go in that direction by supporting uv run tests/runtests.py. Although we might end up with duplicate specifications? idk, something to consider.

yeah, I think it might be quite difficult to keep requirements pins synchronised between our various scripts (and keep the pins up-to-date with renovate) if we have different PEP-723 pins in each script. Call me old-fashioned, but I also just quite like to be able to run scripts with just python rather than uv run :-)

@Avasam
Copy link
Collaborator

Avasam commented Oct 2, 2024

Call me old-fashioned, but I also just quite like to be able to run scripts with just python rather than uv run

Oh yeah, that would have to be optional. I think if Renovate supports it I might seriously consider it, until then I just wanted to at least put the idea out there.

As for whether this breaks my workflow: I very rarely have to call black, ruff, flake8, etc. manually because my editor shows issues in real time and autofixes. (type checkers are different because of our dynamic configs)
I mainly run a full check through runtests.py. If I wanna run black or flake8 directly w/o pre-commit, I'll just manually install it in the venv

After typing a command once it stays in my history (thanks powershell 7), so even long commands are just a few keystrokes after the first time.

@AlexWaygood AlexWaygood merged commit ad96829 into python:main Oct 5, 2024
99 checks passed
@AlexWaygood AlexWaygood deleted the move-lint-requirements branch October 5, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants