-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Move linting to ruff #6966
Move linting to ruff #6966
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefile
Outdated
python3 -c "import isort" > /dev/null 2>&1 || python3 -m pip install isort | ||
python3 -m black --target-version py37 . | ||
python3 -m isort . | ||
ruff --fix . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove Black?
Please also make sure Ruff is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, oversight there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, should this actually be pre-commit run --all-files black ruff
?
@@ -18,10 +18,9 @@ | |||
import tempfile | |||
|
|||
from . import Image, ImageFile | |||
from ._binary import i8 | |||
from ._binary import i8, o8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened here? Does Ruff not support isort's Black profile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isort is a bit looser than Ruff with these; Ruff wants either everything or nothing to be grouped like this, and isort doesn't always care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing, isort always sorts this like the original, with or without the Black profile.
Anyway, I don't mind too much, as long as it remains relatively stable.
On that topic, maybe we should wait until Ruff is at "v1" or "stable", which looks to be close:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruff has Black profile as the ONLY profile possible.
The addition of https://github.com/python-pillow/Pillow/actions/runs/4253283135/jobs/7404119827 |
Ah, should've known there are still uncivilized OSes like that... 😁 Switched to |
Makes sense, we'll probably have to move to |
I don't think there is - Ruff issues don't have the same sort of If you want, I'm not against "belt and suspenders"ing here and keep Bandit also in the configuration... |
Yeah lots not switch, just add ruff … if possible/practical to do so. |
If you do I run a separate GitHub that is superfast (<10 seconds) to quicky add annotations to code changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using the real Bandit directly in pre-commit because it supports severities, so let's remove entirely from Ruff.
I'm curious why the original is so slow, what machine do you have? How do the actual elapsed times compare? |
Ruff linting the CPython codebase from scratch 0.29 sec. |
I was measuring CPU time on purpose – wallclock time isn't affected that much, but the old tooling isn't able to utilize all cores on my machine (macOS 12.6.3, 2,4 GHz 8-Core Intel Core i9, etc.). |
Ah right, still nice but less of a visible benefit. The old pre-commit speed isn't really a problem locally, and on CI it barely makes a difference compared to the full test matrix.
Impressive indeed, although CPython doesn't use Flake8 and this repo isn't CPython :) |
https://beta.ruff.rs/docs/rules/#pygrep-hooks-pgh has four of the 10 hooks from https://github.com/pre-commit/pygrep-hooks:
|
@hugovk Done! I also have another branch that would stack on top of this to switch from .pre-commit-config.yaml | 7 +------
Makefile | 3 +--
Tests/check_jpeg_leaks.py | 7 ++-----
Tests/helper.py | 4 ++--
Tests/test_box_blur.py | 40 ++++++++++++++++++++--------------------
Tests/test_file_jpeg.py | 8 ++------
Tests/test_font_leaks.py | 5 ++++-
Tests/test_image_access.py | 4 +---
Tests/test_image_filter.py | 28 +++++++++++++---------------
setup.py | 9 ++++-----
10 files changed, 50 insertions(+), 65 deletions(-) |
Thanks! Yeah, let's keep this for the linter for now. How does this look with the UP031 fixes? |
The UP031 fixes would involve rewriting the |
@hugovk @radarhere Could this maybe be good to go now? 😁 I could follow up with a PR to remove the six UP031 |
Makefile
Outdated
python3 -m black --target-version py38 . | ||
python3 -m isort . | ||
python3 -c "import ruff" > /dev/null 2>&1 || python3 -m pip install ruff | ||
python3 -m ruff --preview --fix . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If stability is a concern, then wouldn't it be better to not use --preview
? I have to imagine that any features that were in preview back when this PR was created are no longer in preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sans --preview
:
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(LOG was added via #7421.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go for stability and avoid --preview
. Any idea when LOG will leave preview?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really think there's a big huge stability issue here, since whoever ends up updating the Ruff version in this repo will probably look at what they're doing?
Also, the more happy users a preview
rule has, the better its chances are to graduate, I think 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove --preview
, I'd prefer to keep things stable and not spend time double checking potentially-unstable changes every month.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed --preview
, and also the "LOG"
family of lints, because it would otherwise warn about it on every --preview
less invocation (that's not --quiet
ened):
(Pillow) ~/b/Pillow (ruff) $ ruff .
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
warning: Selection `LOG` has no effect because the `--preview` flag was not included.
(Pillow) ~/b/Pillow (ruff) $ ruff --quiet .
(Pillow) ~/b/Pillow (ruff) $
I don't think I have strong opinions on the style changes seen here, so once @hugovk is ok with this, I'm happy for it to be merged. |
c4c9f53
to
ec98fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also delete .flake8
(recently added in #7484).
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com> Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@hugovk Done, rebased, etc. :) |
Thanks, a followup for UP031 would be good. And I'd prefer to keep Black rather than the Ruff formatter. |
## Summary See python-pillow/Pillow#6966 :) ## Test Plan Looked at the Markdown preview!
This PR switches linting (flake8, isort,
bandit, yesqa, pygrep-hooks) for Pillow to theruff
linter.pre-commit
run is much faster (45.15 seconds of CPU time to 8.62 seconds on my machine)ruff
that had been previously enabled isrst-backticks
frompygrep-hooks
.pygrep-hooks
since it enables the wholePGH
family.👉 Instead of touching expressions likeNo longer an issue via Clarify some local variable names #6971.x, y, l, r, w, a, d, f = metrics[ix]
, I opted to adding specificnoqa
directives to quiesce Ruff's "ambiguous name" forl
(which could be an1
or anI
😁)