Python lint: Use ruff instead of flake8. NFC #23085

cclauss · 2024-12-05T12:18:03Z

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.

Set some upper limits on code complexity using ruff rules for McCabe cyclomatic complexity and Pylint.

% git grep flake8
% flake8-to-ruff .flake8 > pyproject.toml
% validate-pyproject pyproject.toml
% pyproject-fmt pyproject.toml
% ruff check

The Required ci/circleci: flake8 job was renamed to ci/circleci: ruff so the Required label should be removed from the old job and added to the new one.

% ruff rule C901

complex-structure (C901)

Derived from the mccabe linter.

What it does

Checks for functions with a high McCabe complexity.

Why is this bad?

The McCabe complexity of a function is a measure of the complexity of
the control flow graph of the function. It is calculated by adding
one to the number of decision points in the function. A decision
point is a place in the code where the program has a choice of two
or more paths to follow.

Functions with a high complexity are hard to understand and maintain.

Example

def foo(a, b, c):
    if a:
        if b:
            if c:
                return 1
            else:
                return 2
        else:
            return 3
    else:
        return 4

Use instead:

def foo(a, b, c):
    if not a:
        return 4
    if not b:
        return 3
    if not c:
        return 2
    return 1

Options

lint.mccabe.max-complexity

sbc100

Sounds like ruff is indeed an improvement over flake8!

Is there any stuff that flake8 does that ruff does now catch?

emcc.py

sbc100 · 2024-12-05T17:09:55Z

test/jsrun.py

@@ -106,7 +106,8 @@ def run_js(filename, engine, args=None,
        stderr=stderr,
        cwd=cwd,
        timeout=timeout,
-        universal_newlines=True)
+        universal_newlines=True,
+        check=skip_check)


This looks like maybe a semantic change.. did ruff make this?

ruff rule PLW1510 will set check=False which ignores failures as recently discussed at https://github.com/pyodide/pyodide-build/pull/66/files#r1861265823 We can set check=False if we always want to ignore errors.

Can we simply revert this?

test/runner.py

sbc100 · 2024-12-05T17:11:59Z

tools/emscripten.py

@@ -368,13 +368,13 @@ def emscript(in_wasm, out_wasm, outfile_js, js_syms, finalize=True, base_metadat
    # check_call doesn't support the `input` argument.
    if asm_consts:
      validate = '\n'.join([f'var tmp = {f};' for _, f in asm_consts])
-      proc = subprocess.run(config.NODE_JS + ['--check', '-'], input=validate.encode('utf-8'))
+      proc = subprocess.run(config.NODE_JS + ['--check', '-'], input=validate.encode('utf-8'), check=False)


Why this? check is false by default isn't it?

The default is to ignore errors.

ruff rule PLW1510

Why is this bad?

By default, subprocess.run does not check the return code of the process
it runs. This can lead to silent failures.

Instead, consider using check=True to raise an exception if the process
fails, or set check=False explicitly to mark the behavior as intentional.

I think just leaving it out is fine/clear.

Can we revert revert this hunk.

sbc100 · 2024-12-05T17:16:04Z

Can we stick with .ruff.toml for now (i.e. a specific config file that is clearly for ruff alone) (at least until we have some other reason to have project.toml)

cclauss · 2024-12-05T20:20:46Z

Is there any stuff that flake8 does that ruff does not catch?

https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-flake8

Can we stick with .ruff.toml for now.

I would have migrated .coveragerc, .mypy.ini and requirements-dev.txt into pyproject.toml in this PR but I know that too many unrelated changes slows the code review process.

PEP 621: Migrate .coveragerc and .mypy.ini into pyproject.toml #23089

sbc100 · 2024-12-05T20:29:47Z

Lets just go with .ruff.toml initially maybe?

sbc100

lgtm % comments

cclauss · 2024-12-09T07:44:18Z

ruff check --ignore=PLW1510
To revalidate these numbers, run ruff check --select=C901,PLR091.

Below, the Required ci/circleci: flake8 job was renamed to ci/circleci: ruff so the Required label should be removed from the old job and added to the new one.

cclauss · 2024-12-09T09:09:53Z

This PR will not auto-merge because the Required ci/circleci: flake8 job no longer exists.

sbc100 · 2024-12-13T01:05:16Z

@cclauss it looks like lost the check for the correct number of newlines between methods and functions: https://github.com/emscripten-core/emscripten/pull/23139/files#r1883095264

cclauss · 2024-12-13T07:50:35Z

https://github.com/emscripten-core/emscripten/pull/23139/files#r1883095264

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](https://pypi.org/project/flake8/) (plus dozens of plugins), [Black](https://github.com/psf/black), [isort](https://pypi.org/project/isort/), [pydocstyle](https://pypi.org/project/pydocstyle/), [pyupgrade](https://pypi.org/project/pyupgrade/), [autoflake](https://pypi.org/project/autoflake/), and more, all while executing tens or hundreds of times faster than any individual tool.

sbc100 · 2025-01-07T00:43:08Z

It looks like we also regressed on the ability to detect trailing whitespace in function calls:

$ cat test.py 
print(1, 2 )
$ ruff check test.py
All checks passed!
$ flake8 test.py
test.py:1:11: E202 whitespace before ')'

Is this just something we need to enable?

sbc100 · 2025-01-07T00:47:44Z

I guess E202 is in preview too: https://docs.astral.sh/ruff/rules/whitespace-before-close-bracket/. Somewhat worrying that such simple checks are in preview.. and that we have to enable them explicitly on the command line.

cclauss · 2025-01-07T04:30:14Z

This is solved in #23154.

% echo "print(1, 2 )" | pipx run ruff format -

print(1, 2)

Flake8 was created before great code formatters were available but Ruff was not.
I don't think the Ruff Team puts too many cycles into rules that are better solved by black or ruff format.

sbc100 · 2025-01-07T17:03:30Z

It would have been good to know that before we made the transition to ruff.

Upgrade to ruff v0.9.1 with 2025 stable format. Required no code changes. https://github.com/astral-sh/ruff/releases Fix #23085 (comment) Add https://docs.astral.sh/ruff/rules/#flake8-pie-pie Add [`ruff rule PLR1714`](https://docs.astral.sh/ruff/rules/repeated-equality-comparison)

Turns out ruff still doesn't support several flake8 warnings. I've enabled some more that are in preview-only mode. See emscripten-core#23085

Turns out ruff still doesn't support several flake8 warnings. I've enabled some more that are in preview-only mode. See #23085

sbc100 reviewed Dec 5, 2024

View reviewed changes

cclauss force-pushed the ruff branch from f198e08 to e8412d7 Compare December 5, 2024 20:20

cclauss force-pushed the ruff branch 4 times, most recently from b43b435 to 71f966f Compare December 7, 2024 09:28

cclauss requested a review from sbc100 December 8, 2024 12:04

sbc100 approved these changes Dec 9, 2024

View reviewed changes

Python lint: Use ruff instead of flake8

2d9e441

cclauss force-pushed the ruff branch from fc57e74 to 2d9e441 Compare December 9, 2024 07:41

sbc100 enabled auto-merge (squash) December 9, 2024 08:03

sbc100 changed the title ~~Python lint: Use ruff instead of flake8~~ Python lint: Use ruff instead of flake8. NFC Dec 9, 2024

sbc100 disabled auto-merge December 9, 2024 16:42

sbc100 merged commit 19345a7 into emscripten-core:main Dec 9, 2024
28 checks passed

cclauss deleted the ruff branch December 9, 2024 16:42

cclauss mentioned this pull request Jan 10, 2025

Upgrade to ruff v0.9.1 and add PIE ruff rules. NFC #23362

Merged

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 14, 2025

Fix flake8 errors/warnings. NFC

ee65acd

Turns out ruff still doesn't support several flake8 warnings. I've enabled some more that are in preview-only mode. See emscripten-core#23085

sbc100 mentioned this pull request Mar 14, 2025

Fix flake8 errors/warnings. NFC #23930

Merged

dschuff mentioned this pull request Mar 14, 2025

Temporarily disable some tests #23933

Merged

sbc100 added a commit that referenced this pull request Mar 17, 2025

Fix flake8 errors/warnings. NFC (#23930)

20b19b2

Turns out ruff still doesn't support several flake8 warnings. I've enabled some more that are in preview-only mode. See #23085

dschuff mentioned this pull request Mar 29, 2025

Revert "Temporarily disable some tests (#23933) #24011

Merged

Python lint: Use ruff instead of flake8. NFC #23085

Python lint: Use ruff instead of flake8. NFC #23085

Conversation

cclauss commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

complex-structure (C901)

What it does

Why is this bad?

Example

Options

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

cclauss Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

cclauss Dec 5, 2024

Choose a reason for hiding this comment

Why is this bad?

Uh oh!

sbc100 Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Dec 5, 2024

Uh oh!

cclauss commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Dec 5, 2024

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

cclauss commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss commented Dec 9, 2024

Uh oh!

Uh oh!

sbc100 commented Dec 13, 2024

Uh oh!

cclauss commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jan 7, 2025

Uh oh!

sbc100 commented Jan 7, 2025

Uh oh!

cclauss commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jan 7, 2025

Uh oh!

Uh oh!

cclauss commented Dec 5, 2024 •

edited

Loading

cclauss Dec 5, 2024 •

edited

Loading

cclauss commented Dec 5, 2024 •

edited

Loading

cclauss commented Dec 9, 2024 •

edited

Loading

cclauss commented Dec 13, 2024 •

edited

Loading

cclauss commented Jan 7, 2025 •

edited

Loading