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

Replace isort with ruff check and black with ruff format #75

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Dec 14, 2024

PR Goal?

Streamline Python linting and formatting.

https://docs.astral.sh/ruff is an extremely fast Python linter and code formatter, written in Rust. With over 800 linting rules, ruff can be used to replace Flake8 (plus dozens of plugins), Black, isort, pydocstyle, pyupgrade, autoflake, and more, all while executing tens or hundreds of times faster than any individual tool.

Add linting tests for bugbear, comprehensions, code complexity, pylint, etc.

Fixes?

Feedback sought?

Priority?

Tests added?

How to test?

Confidence?

Version change?

Copy link
Contributor

@dhdaines dhdaines left a comment

Choose a reason for hiding this comment

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

Yes, I like ruff quite a bit. Looks like it isn't conflicting with the existing formatting (and also finds an error). As long as CI passes I say good!

from ._soundswallower import Endpointer # noqa: F401
from ._soundswallower import FsgModel # noqa: F401
from ._soundswallower import Vad # noqa: F401
from ._soundswallower import ( # noqa: F401
Copy link
Member

@joanise joanise Dec 16, 2024

Choose a reason for hiding this comment

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

We don't need noqa: F401 here: all these names are used in __all__, so they're not "imported but unused".

Removing it and re-running isort collapses this import statement to just one line, and I assume (and hope!) ruff will do the same.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, looking at the history, we did need F401 at some point, just not anymore.

Copy link
Contributor Author

@cclauss cclauss Dec 16, 2024

Choose a reason for hiding this comment

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

There is a rule for that... RUF100. https://docs.astral.sh/ruff/rules/unused-noqa/

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I had forgotten all about __all__ and gotten out of the habit of using it, thanks for the reminder!

Copy link
Member

Choose a reason for hiding this comment

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

I tend to avoid __all__ in general, just like I never use import *, but in a top-level situation like this, where you're importing stuff into __init__.py just so others can do from soundswallower import foo, putting those names into __all__ as you have done is the right thing to do.

@joanise
Copy link
Member

joanise commented Dec 16, 2024

If I deliberately disorder the imports and a file and run pre-commit run --all-files, I get an error but the file is not actually fixed. isort and black were configured to auto fix, ruff needs to be configured to do the same for formatting issues.
It's appropriate for ruff to just show errors and fail pre-commit for flake8 issues, but not for isort and black issues.

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

See previous comments.

Comment on lines +14 to +16
- id: ruff
args: [ --fix ]
- id: ruff-format
Copy link
Member

Choose a reason for hiding this comment

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

Much better, thank you! We can reduce noise by reverting these lines:

Suggested change
- id: ruff
args: [ --fix ]
- id: ruff-format
- id: ruff-format
- id: ruff
args: [ --fix ]

While the end result is effectively the same, things that are fixed by ruff-format don't get reported as errors by ruff in this order, but they do in the other order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please read https://docs.astral.sh/ruff/integrations/#pre-commit

When running with --fix, Ruff's lint hook should be placed before Ruff's formatter hook...

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I guess it's a bit of a chicken and egg problem: ruff-format fixes things ruff complains about, but also stuff ruff --fix introduces, I guess I can live with the duplicate warnings.

Copy link
Member

@joanise joanise 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 now, thanks!

@joanise joanise merged commit 43defa4 into ReadAlongs:main Dec 16, 2024
21 checks passed
@cclauss cclauss deleted the ruff branch December 16, 2024 19:33
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.

3 participants