-
Notifications
You must be signed in to change notification settings - Fork 234
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
chore: use Ruff #1405
chore: use Ruff #1405
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.
Looks cool, fewer tools is better. And indeed it is crazy fast. Makes it much more suitable for a pre-commit hook.
cibuildwheel/options.py
Outdated
image = ( | ||
pinned_images.get(config_value, config_value) | ||
if config_value | ||
else pinned_images["manylinux2014"] | ||
) |
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 my only quibble in this PR is this refactor (and the similar one below). The old version reads clearly, step-by-step, the new one is peculiar, I find it hard to read the intent of the code.
I checked the previous code locally and ruff doesn't seem to disagree with it so maybe the rule has since been ignore'd.
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.
Maybe old ruff was used. SIM108 was changed to not trigger if the new code will not fit in a single line. astral-sh/ruff#1802
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.
Yes, old, ancient ruff was used. :P
I'll rerender this later with newer Ruff. Don't want to wipe my usefixtures changes.
@@ -460,7 +460,7 @@ def build(options: Options, tmp_path: Path) -> None: | |||
env=env, | |||
capture_stdout=True, | |||
).strip() | |||
testing_archs: list[Literal["x86_64", "arm64"]] | |||
testing_archs: list[Literal["x86_64", "arm64"]] # noqa: F821 |
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.
This one confused me, but it seems like a bug in ruff, I opened a report here - astral-sh/ruff#2105 .
I do wonder if we need F821 at all, given that mypy will always catch this error anyway.
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 feel free to keep filing stuff as questions and issues come up. Glad we could resolve this one :))
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.
It's not resolved, it's just a duplicate of an existing issue (astral-sh/ruff#2005) you helped me open a couple of days ago. :)
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.
It will go away (Ruff will actually remove it) when this does get fixed, so fine to leave the noqa in here. But we could probably turn off F821 as well, since we are fully typed. It's probably mostly useful if you have code not checked by MyPy.
What is need to be done here? |
Hmm, maybe it's just on me to rerender? |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
6857bd5
to
df1f793
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.
looks good!
Ruff readme consistent with real state :) |
Oh sorry, that was sent in a PR, I should've checked first! |
We fixed it on our side 😛 |
Is 10-100x faster (written in Rust), combines flake8 and lots of plugins, pyupgrade, isort, pygrep, yesqa, and some of flake8 (so pre-commit setup is faster - also just a simple binary, I think). And pyproject.toml configured! Also can do auto-fixes for quite a few errors. Ran into a few minor bugs that have already been fixed upstream. Used by Pandas, FastAPI, SciPy, Build, etc. More pre-commit friendly, since we don't have breakage due to plugins not being pinned (because they are all built-in, so there's only the one pre-commit ref).
Suggested in #1399 (comment).