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

flake8 → ruff? #628

Closed
DimitriPapadopoulos opened this issue Dec 31, 2023 · 9 comments · Fixed by #629 or #651
Closed

flake8 → ruff? #628

DimitriPapadopoulos opened this issue Dec 31, 2023 · 9 comments · Fixed by #629 or #651

Comments

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Dec 31, 2023

How about changing flake8 to ruff? While I wasn't interested initially, ruff does gain momentum, and beyond speed, it replaces all Python linters by a single tool, and is less opinionated than say black.

These files would be slightly modified:

The only change in code suggested using the default settings of ruff:

In addition to being user as a linter, ruff may be used as a formatter, as a less opinionated alternative to black (single quotes are OK).

Finally, it would tick a box in the issues reported by the Repo-Review of Scientific Python:

  • RF001: Has Ruff config
    Must have [tool.ruff] section in pyproject.toml or ruff.toml/.ruff.toml.
@adrienverge
Copy link
Owner

adrienverge commented Jan 4, 2024

Interesting!

If I understand correctly, the only new errors reported by ruff are fixed by your PR #627.
But are there any errors reported by flake8, that ruff doesn't detect? E.g. the import lines order?

EDIT: I just saw your PR #629.

@DimitriPapadopoulos
Copy link
Contributor Author

Indeed, the only required change is #627.

The rest of the changes is because I chose to enable the non-default isort (I) rules, to obtain the equivalent of isort. I thought of it as a nice illustration of what ruff can do. You are free to disagree, or defer to a different PR - I already have two distinct commits.

@adrienverge
Copy link
Owner

You are free to disagree, or defer to a different PR - I already have two distinct commits.

I completely agree!

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jan 4, 2024

My wrong, the reason I added isort (I) rules is that we need a replacement for flake8-import-order, in addition to plain flake8.

@adrienverge
Copy link
Owner

adrienverge commented Jan 31, 2024

I assumed that ruff superseded flake8, meaning that all previous checks were still enforced. This doesn't seem to be the case. For example after editing a few files:

$ ruff .
# nothing
$ flake8 .
./tests/rules/test_indentation.py:195:80: E501 line too long (81 > 79 characters)
./tests/rules/test_truthy.py:52:21: E225 missing whitespace around operator
./tests/test_spec_examples.py:45:1: E302 expected 2 blank lines, found 1
./yamllint/rules/truthy.py:150:1: E265 block comment should start with '# '
./yamllint/rules/truthy.py:152:1: E265 block comment should start with '# '
./yamllint/rules/truthy.py:178:13: E265 block comment should start with '# '
./yamllint/rules/truthy.py:181:13: E265 block comment should start with '# '
./yamllint/rules/truthy.py:185:80: E501 line too long (80 > 79 characters)
./yamllint/rules/truthy.py:186:17: E131 continuation line unaligned for hanging indent

@DimitriPapadopoulos are you aware of that?

@DimitriPapadopoulos
Copy link
Contributor Author

See How does Ruff's linter compare to Flake8? for details:

Under those conditions, Ruff implements every rule in Flake8. In practice, that means Ruff implements all of the F rules (which originate from Pyflakes), along with a subset of the E and W rules (which originate from pycodestyle).

E501

The default line-length is 88. I can change it to be 79 like in flake8, but 79 is really short and text terminals with 80 columns are out of vogue nowadays.

E131, E225, E265

I cannot reproduce most of the flake8 errors in the current master branch:

$ flake8 ./tests/rules/test_truthy.py ./tests/test_spec_examples.py
./tests/test_spec_examples.py:45:1: E302 expected 2 blank lines, found 1
$ 

E302

E302 is indeed not part of the E subset. The flake8 error can be reproduced easily:

$ cat /path/to/file.py
from foobar import FooBar

class SpecificationTestCase(FooBar):
    rule_id = None
$ 
$ flake8 /path/to/file.py
/path/to/file.py:3:1: E302 expected 2 blank lines, found 1
$ 
$ ruff /path/to/file.py
$

This is the first time I actually notice a flake8 rule that has an impact on readability missing from ruff. I guess ruff considers this rule is not relevant for linters, but formatters. Indeed, the ruff and black formatters do fix this issue:

$ cat /path/to/file.py
from foobar import FooBar

class SpecificationTestCase(FooBar):
    rule_id = None
$ 
$ ruff format /path/to/file.py
1 file reformatted
$ 
$ cat /path/to/file.py
from foobar import FooBar


class SpecificationTestCase(FooBar):
    rule_id = None
$ 
$ cat /path/to/file.py
from foobar import FooBar

class SpecificationTestCase(FooBar):
    rule_id = None
$ 
$ black /path/to/file.py
reformatted /path/to/file.py
All done! ✨ 🍰 ✨
1 file reformatted.
$ 
$ cat /path/to/file.py
from foobar import FooBar


class SpecificationTestCase(FooBar):
    rule_id = None
$ 

@adrienverge
Copy link
Owner

All these differences weren't clear to me in #628 and #629. Last month I asked "But are there any errors reported by flake8, that ruff doesn't detect?" and I understood that the answer was "no" (except for import lines order, but that was fixed elsewhere). Sorry about that.

In reality the answer seems to be "yes": obviously ruff is not a drop-in replacement for flake8.

I cannot reproduce most of the flake8 errors in the current master branch:

The example I posted was "after editing a few files". But this can be reproduced easily:

A=1 #comment

def f(a, b
          , c,d):
    f(1
       )
$ ruff demo.py
# nothing
$ flake8 demo.py
demo.py:1:2: E225 missing whitespace around operator
demo.py:1:4: E261 at least two spaces before inline comment
demo.py:1:5: E262 inline comment should start with '# '
demo.py:3:1: E302 expected 2 blank lines, found 1
demo.py:3:11: E203 whitespace before ','
demo.py:4:11: E127 continuation line over-indented for visual indent
demo.py:4:15: E231 missing whitespace after ','
demo.py:6:8: E124 closing bracket does not match visual indentation

The default line-length is 88. I can change it to be 79 like in flake8, but 79 is really short and text terminals with 80 columns are out of vogue nowadays.

The default line-length of Python code in yamllint could be discussed, but same: it's too bad that this change wasn't explicit in the pull request. For the time being I prefer that we stick to 79.

Anyway, ruff doesn't seem to enforce it 🤔, for example with the following change, ruff yamllint/cli.py doesn't output anything (and same if I add line-length = 79 in .ruff.toml):

@@ -161,2 +161 @@ def run(argv=None):
-    parser.add_argument('--list-files', action='store_true', dest='list_files',
-                        help='list files to lint and exit')
+    parser.add_argument('--list-files', action='store_true', dest='list_files', help='list files to lint and exit')

I'm considering reverting all the ruff-related changes. Do you see an alternative to keep all the checks enforced by flake8?

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Feb 1, 2024

Indeed, it appears ruff is not a drop-in replacement for flake8 if you don't use a formatter. I hadn't noticed that in other projects, as most of them embrace formatters - at least in the scientific Python ecosystem. I think it would be an error to avoid change, but I can submit a PR:

Edit: The changes in #651 should be sufficient.

@adrienverge
Copy link
Owner

Sound good. Thanks a lot for handling this quickly @DimitriPapadopoulos 🙏

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