-
Notifications
You must be signed in to change notification settings - Fork 37
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 flake8 with ruff linter #590
Conversation
e1be14d
to
75287f6
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.
Never thought, that one day I will replace flake8
Just a quick note that ruff's handling of ANN401 still does contain a bug, see: astral-sh/ruff#5803 so we might want to hold this off until that's resolved, or disable the enforcement of this rule entirely, as it wasn't enforced by It is however a pretty minor bug, only concerning overloads, otherwise the implementation works exactly the same as it would with Another note I'd mention on ruff's handling of ANN401 is that, as I said in one of the requested changes, ruff doesn't consider the use of Any for variables an issue, and the use of Any in generic variables is also not an issue. This is the same behavior as |
I don't think this is a big problem for us, so we can just ignore it for now. |
This replaces
flake8
in favor ofruff
, which is a linter written in rust, and runs much faster (10-100x speedup).It has first party support for all of the flake8 extensions which we were using (except for
flake8-new-union-types
, but there is an equivalent rule in thepyupgrade
rule set). It also supports configuration directly frompyproject.toml
, meaning we lo longer need yet another config file in the project's root (.flake8
).Ruff is almost completely compatible with flake8, so no breaking changes in code style are introduced here.
Amazingly enough, ruff also has
--fix
argument, which can be used to automatically fix any rule violations that it knows how to fix. This can end up greatly reducing a lot of manual labor, especially for new contributors not used to our linting rules.I also enabled
RUF
ruleset, which contains various custom rules created by the ruff devs. Generally, these are pretty useful common sense rules and we currently don't violate almost anything there, except for these:RUF012
, which enforces that class variables which are mutable should use thetyping.ClassVar
annotation is violated in some places. For now, I simply ignored this rule, however we can enable it later if we'd like, and fix the violations in another PR.RUF100
, which is a rule about unnecessary noqa comments. All violations of this rule were fixed here. Note that it might sometimes seem like the noqa comment should be there, and it might seem like a bug that ruff doesn't find an error there.I've added explanations for these as review comments, pointing out why they were removed, but the general reason is that ruff is just really smart, and understands when a rule should not apply. I marked these as resolved though, so they won't prevent a merge (as our branch protection doesn't allow unresolved conversations), so you'll need to click over my comments there to read them.