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

infra: replace flake8 with ruff #565

Merged
merged 5 commits into from
Jan 22, 2023
Merged

infra: replace flake8 with ruff #565

merged 5 commits into from
Jan 22, 2023

Conversation

layday
Copy link
Member

@layday layday commented Jan 20, 2023

As a first step, this ports all of our existing flake8 rules to Ruff. I have omitted pyupgrade for which Ruff has partial support.

As a first step, this ports all of our existing flake8 rules to Ruff.
I have omitted pyupgrade for which Ruff has partial support.
@henryiii
Copy link
Contributor

henryiii commented Jan 20, 2023

You beat me (by about a day). I've already started on scikit-build-core and cibuildwheel. :)

@@ -124,7 +124,7 @@ def test_build(monkeypatch, project, args, call, tmp_path):

def test_isolation(tmp_dir, package_test_flit, mocker):
try:
import flit_core # noqa: F401
import flit_core # noqa: F401 # imported but unused
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, would import flit_core as _ work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the correct way to do this would be checking importlib.util.find_spec("flit_core") against None.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just curious, would import flit_core as _ work?

It does not.

Copy link
Member

Choose a reason for hiding this comment

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

Could try adding del flit_core to the else-block, then.

Copy link
Contributor

Choose a reason for hiding this comment

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

importlib.util.find_spec("flit_core") is the correct solution imo. It’s also faster if the module is large, since you don’t have to load it. Not that important for tests, but nice to do things the right way as a good example.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you wouldn’t need an else block for the del. It’s never used, so it could be in the try.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd just need to reference it; flit_core on a new line would be sufficient. I don't think we need to do any of those things however, silencing the warning where it does not apply is perfectly fine.

Copy link
Member

Choose a reason for hiding this comment

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

@henryiii yep, using importlib is certainly better, but I was suggesting to use the fact that the else-block is already present, w/o much refactoring.
Also, referencing it in the try-section is a code smell because it should always have exactly one instruction.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

Ruff can also replace pyupgrade (the support is nearly there, only 1-2 fixes not supported - I think it's fine for a continuous check), isort, and the Python parts of the pygrep hooks. No need to do that right now, but would be a good followup.

@layday layday merged commit 0981519 into pypa:main Jan 22, 2023
@layday layday deleted the switch-to-ruff branch January 22, 2023 10:35
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.

None yet

4 participants