Skip to content

gh-110558: Run ruff on Argument Clinic in CI #110559

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

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Oct 9, 2023

@AlexWaygood
Copy link
Member Author

@AlexWaygood
Copy link
Member Author

A few questions:

  1. Should we also run all pyflakes checks on Lib/test/test_clinic.py? I'd quite like to; we also had things like unused variables etc. in the test file in the past. If so, maybe we should remove that file from the existing ruff check? (But having it manually excluded from the existing ruff check would add complexity, and ruff is incredibly fast, so maybe it's okay to run ruff twice in CI on that one file.)
  2. Do we want to add ruff to Tools/requirements-dev.txt? It might be helpful for running ruff locally -- you could just run pip install -r Tools/requirements-dev.txt in a venv to install all the linting tools for Argument Clinic. On the other hand, we're running ruff via pre-commit in CI, and it's really easy to do that locally as well, so maybe it's not necessary to add it to Tools/requirements-dev.txt.
  3. Any other rules we want to add right off the bat? (We can always add more in follow-up PRs.)

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 9, 2023

  1. Should we also run all pyflakes checks on Lib/test/test_clinic.py? I'd quite like to; we also had things like unused variables etc. in the test file in the past. If so, maybe we should remove that file from the existing ruff check? (But having it manually excluded from the existing ruff check would add complexity, and ruff is incredibly fast, so maybe it's okay to run ruff twice in CI on that one file.)

On the one hand, a nicer way of solving this might be to work on slowly adding more enabled error codes to https://github.com/python/cpython/blob/main/Lib/test/.ruff.toml. On the other hand, there are currently 3095 errors reported by ruff if you enable all the pyflakes rules and run it on Lib/test/, whereas there are 0 errors reported by ruff if you do the same thing, but run it only on Lib/test/test_clinic.py. So it would probably be quite a while before we'd be able to enable all the pyflakes checks on all of Lib/test/...

@erlend-aasland
Copy link
Contributor

  1. Do we want to add ruff to Tools/requirements-dev.txt? It might be helpful for running ruff locally -- you could just run pip install -r Tools/requirements-dev.txt in a venv to install all the linting tools for Argument Clinic. On the other hand, we're running ruff via pre-commit in CI, and it's really easy to do that locally as well, so maybe it's not necessary to add it to Tools/requirements-dev.txt.

Running ruff via pre-commit sounds good to me; having more such stuff in pre-commit is convenient and useful.

@AlexWaygood AlexWaygood added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 10, 2023
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 10, 2023

I'm going to try backporting this, but if ruff fails for any reason on the older branches, I'm not going to try too hard to fix it (I'll just close the PRs).

@AlexWaygood AlexWaygood enabled auto-merge (squash) October 10, 2023 07:38
@AlexWaygood AlexWaygood merged commit 7b2764e into python:main Oct 10, 2023
@AlexWaygood AlexWaygood deleted the ruff-arg-clinic branch October 10, 2023 07:52
@miss-islington
Copy link
Contributor

Thanks @AlexWaygood for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @AlexWaygood, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7b2764e798e400b8f5fcc199739405e6fbd05c20 3.12

@miss-islington
Copy link
Contributor

Sorry, @AlexWaygood, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7b2764e798e400b8f5fcc199739405e6fbd05c20 3.11

@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2023

GH-110598 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 10, 2023
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Oct 10, 2023
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Oct 10, 2023

I created a backport PR to 3.12, but there's a ton of ruff failures on 3.11, so I won't bother with that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants