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

chore: use Ruff #1405

Merged
merged 1 commit into from
Jan 30, 2023
Merged

chore: use Ruff #1405

merged 1 commit into from
Jan 30, 2023

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 20, 2023

  • chore: use ruff
  • chore: use ruff (manual changes)

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).

test/test_pure_wheel.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@joerick joerick left a 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.

unit_test/main_tests/main_options_test.py Show resolved Hide resolved
Comment on lines 558 to 562
image = (
pinned_images.get(config_value, config_value)
if config_value
else pinned_images["manylinux2014"]
)
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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.

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 :))

Copy link
Contributor Author

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. :)

Copy link
Contributor Author

@henryiii henryiii Jan 23, 2023

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.

@Czaki
Copy link
Contributor

Czaki commented Jan 30, 2023

What is need to be done here?

@henryiii
Copy link
Contributor Author

Hmm, maybe it's just on me to rerender?

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

looks good!

@henryiii henryiii merged commit c8b037b into pypa:main Jan 30, 2023
@henryiii henryiii deleted the henryiii/chore/ruff branch January 30, 2023 20:53
@Czaki
Copy link
Contributor

Czaki commented Jan 30, 2023

Ruff readme consistent with real state :)
(cibuildwheel was already mentioned as a project that uses Ruff)

@charliermarsh
Copy link

charliermarsh commented Jan 30, 2023

Oh sorry, that was sent in a PR, I should've checked first!

@Czaki
Copy link
Contributor

Czaki commented Jan 30, 2023

Oh sorry, that was sent in a PR, I should've checked first!

We fixed it on our side 😛

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

Successfully merging this pull request may close these issues.

4 participants