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

checks(ruff): Configure ruff linter #3972

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Conversation

echoix
Copy link
Member

@echoix echoix commented Jul 2, 2024

Ruff is a python linter written in rust, that is extremely fast. https://docs.astral.sh/ruff/

This PR adds ruff configuration to pyproject.toml, enabling all rules that aren't currently failing.

Some rules support fixes, so ruff is added to the python-code-quality workflow with suggestions.
Ruff is also added to pre-commit, with fixes.

I made sure to ignore every rule that was failing, as well as adding some per-file-ignores for some rules where I really didn't want to wait enabling them, or not many files were failing. This way, to fix some new rules, look at an ignored rule, look https://docs.astral.sh/ruff/rules/ to see if fixes are available, and remove the ignored identifier.

# To fix
ruff check --fix
# to also enable rules that are in preview
ruff check --fix --preview
# For more fixes, that might need some review
ruff check --output-format=concise --preview --unsafe-fixes --fix

Enabling ruff in the repo like this ensures that the recent fixes will stay fixed, and that the code quality, with respect to of these rules, will not degrade.

@echoix echoix added this to the 8.5.0 milestone Jul 2, 2024
@echoix echoix requested a review from ninsbl July 2, 2024 04:00
@github-actions github-actions bot added CI Continuous integration Python Related code is in Python labels Jul 2, 2024
pyproject.toml Show resolved Hide resolved
Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, quite a list of per-file-exceptions. I guess it took some time to compile that...

@echoix
Copy link
Member Author

echoix commented Jul 3, 2024

Not that much, for the per-file ignores. But the rest ya, a little bit. Yesterday at the middle of the afternoon I was finishing up with the grouping of imports, and tried to reconcile the ruff and isort configurations, and that's when I found out that isort might have to be better configured, so right away I placed the import sort+group PRs as drafts. (Only the simple low-risk one can go through without problems, as I only kept the changes mostly inside the same line rather than the groups themselves. I tried a lot fiddling between running from the root of the repo vs running from the gui/wxpython folder and kept having really different results. It doesn't help that our structure doesn't play well to detecting what is a first party package vs not, as it's not easily importable without workarounds.

It ended up a bit inconclusive (I desespared a bit), so since I already had the 1 isort rule as ruff, why not add the other ones that just got fixed, to make sure the same errors don't get introduced. That was near 16h-17h. After initially going by listing the rules that were fixed by my PRs, seing what other rules worked too, I went all in a 7h nonstop sprint of adding all categories (rule prefixes) one by one, and ignoring the rules that have any failures. I passed through most of the complete list of rules in the ruff docs. Let's say the keyboard keys Alt+Tab, Ctrl + C, Ctrl+V, and Up + Enter got used a lot, plus typing in all the ignored rules.

  • For the per-file-ignores, it is mostly two kinds of rules, that I wanted them to be enabled as soon as possible to not have any more of these errors, like having a string formatted with values before translating it rather than the other way around. This and a couple important ones (to me).

Overall, I decided to have it substractive (rather than additive), and do all the work up front of going through all what exists, and hopefully more in the future, rather than hoping that new rules will be added as time goes by. By seeing how fast the flake8 per-file-ignores are fixed (counting in years), taking the state of the repo now, and not making it worse (for the rules that have no errors now), is way better.

It will be so easy, with how it is set up, to simply delete a line in the ignore list (ideally starting with ones that have auto fix), and have ruff apply the fix and commit them, or even use the CI to upload the changes if you aren't able to do pip install ruff.

@echoix
Copy link
Member Author

echoix commented Jul 3, 2024

I reran a pass of CI with the 4 new commits to see if there are any new errors on main, and there aren't. So I don't need to update this branch from main.

@echoix echoix merged commit 277a147 into OSGeo:main Jul 3, 2024
27 checks passed
@echoix echoix deleted the configure-ruff branch July 3, 2024 03:56
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants