adrienverge / yamllint

A linter for YAML files.
GNU General Public License v3.0
2.9k stars 278 forks source link

flake8 → ruff? #628

Closed DimitriPapadopoulos closed 10 months ago

DimitriPapadopoulos commented 11 months ago

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:

adrienverge commented 10 months ago

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 https://github.com/adrienverge/yamllint/pull/629.

DimitriPapadopoulos commented 10 months ago

Indeed, the only required change is https://github.com/adrienverge/yamllint/pull/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 commented 10 months ago

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

I completely agree!

DimitriPapadopoulos commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 :thinking:, 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 commented 10 months ago

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 commented 9 months ago

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