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 flake8 with ruff linter #590

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Replace flake8 with ruff linter #590

merged 3 commits into from
Aug 2, 2023

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Jul 16, 2023

This replaces flake8 in favor of ruff, 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 the pyupgrade rule set). It also supports configuration directly from pyproject.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 the typing.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.

@ItsDrike ItsDrike force-pushed the ruff-linter branch 2 times, most recently from e1be14d to 75287f6 Compare July 16, 2023 09:45
@ItsDrike ItsDrike marked this pull request as ready for review July 16, 2023 10:01
Copy link
Member

@PerchunPak PerchunPak left a 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

pyproject.toml Show resolved Hide resolved
tests/status_response/__init__.py Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@ItsDrike
Copy link
Member Author

ItsDrike commented Jul 31, 2023

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 flake8-annotations before at all.

It is however a pretty minor bug, only concerning overloads, otherwise the implementation works exactly the same as it would with flake8-annotations.

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 flake8-annotations, but it is potentially a bit unexpected. There is a feature request for this already: astral-sh/ruff#5871 and there will eventually likely be new rules added to address these too.

@PerchunPak
Copy link
Member

I don't think this is a big problem for us, so we can just ignore it for now.

@ItsDrike ItsDrike merged commit 6e9db58 into master Aug 2, 2023
@ItsDrike ItsDrike deleted the ruff-linter branch August 2, 2023 00:14
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