chore: move to Ruff and add rules #4483

henryiii · 2023-02-01T19:27:12Z

Description

Moves the linters over to a single tool - Ruff. This combines a lot of other tools, is 10-100x faster (Rust rewrite), has auto-fix support for some rules, is a single no-dependency binary, and is configured in pyproject.toml. Build, scikit-build-core, Pandas, SciPy, cibuildwheel, FastAPI, and others have switched now too.

Adding a lot more rules since plugins are "free", basically, they don't complicate the environment setup, they are all compiled in.

Ruff technically only supports Python 3.7+, but I think it's fine and we are about to drop 3.6 support too anyway. (Flake8 only supports running on 3.8+ now anyway, too)

Suggested changelog entry:

* Moved the linting framework over to Ruff.

rwgk

Not a review, really ... Looks like a big step forward in almost automatic source code cleanup!

Did you already look at the many CI failures? Is that something we should tackle as a team, divvying up the work?

rwgk · 2023-02-01T20:11:34Z

tests/conftest.py

-        else:
-            self.explanation = _make_explanation(a.splitlines(), b.splitlines())
-            return False
+        self.explanation = _make_explanation(a.splitlines(), b.splitlines())


rwgk · 2023-02-01T20:12:15Z

tests/test_buffers.py

@@ -54,7 +54,8 @@ def test_to_python():
    mat2 = np.array(mat, copy=False)
    assert mat2.shape == (5, 4)
    assert abs(mat2).sum() == 11
-    assert mat2[2, 3] == 4 and mat2[3, 2] == 7
+    assert mat2[2, 3] == 4


henryiii · 2023-02-17T21:35:02Z

tests/test_sequences_and_iterators.py

@@ -72,13 +72,13 @@ def test_iterator_referencing():
    vec = m.VectorNonCopyableIntPair()
    vec.append([3, 4])
    vec.append([5, 7])
-    assert [int(x) for x in vec.keys()] == [3, 5]


This check is kind of often wrong. I've had it trigger incorrectly in another package too. Not all objects play nice and have iter(x) == iter(x.keys()). I wish it was type-aware.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii · 2023-02-18T02:51:28Z

Passing now.

rwgk

Passing now.

AWESOME!

I could only glance through right now. I need to find a block of time to comb through more carefully. Asap.

rwgk · 2023-02-18T19:15:12Z

pyproject.toml

+  "unused-argument",  # covered by Ruff ARG
+]
+
+[tool.ruff]


Is there a ruff doc we could link to from here?
To jump-start finding the answers to: what does this do and why do we need it?
(Only if there is an easy direct link and it makes sense to you.)

https://beta.ruff.rs/docs/

Don't think we need to link to everything everywhere, and that URL might change in the future. I added a note in .pre-commit-config.yaml. Ruff is easy to find via Googling.

rwgk

I looked more carefully now. Overall I'm still amazed, but I'm a worried by the changes ruff made to test_enum.py. Are there more like that elsewhere that I overlooked? I'll try to make another pass through later. I'll also try to get a coverage analysis before and after these changes.

tests/conftest.py

rwgk · 2023-02-20T23:40:00Z

tests/conftest.py

 def msg():
    """Sanitize messages and add custom failure explanation"""
    return SanitizedString(_sanitize_message)


-# noinspection PyUnusedLocal
-def pytest_assertrepr_compare(op, left, right):
+def pytest_assertrepr_compare(op, left, right):  # noqa: ARG001


I tend to use

del op del right

as a way to document what is (obviously) intentionally unused, rather than using suppressions.

I prefer the suppression. It's in the definition, rather than in the implementation, which IMO is better. I shouldn't have to look in the body of the function to be warned that some arguments are ignored. It's also possible these are never called by name in the hook, in which case _op, _right would be better.

rwgk · 2023-02-20T23:52:47Z

tests/test_enum.py

@@ -66,20 +66,20 @@ def test_unscoped_enum():
    # Unscoped enums will accept ==/!= int comparisons
    y = m.UnscopedEnum.ETwo
    assert y == 2
-    assert 2 == y
+    assert y == 2


All, or maybe almost all, changes in this file undermine the original intent in a critical way.

Is there a way to tell ruff not to touch any of the operator tests in this file?

Generally: the requirements for production code are subtly and critically different from requirements for unit test code. Does ruff have a concept of the two classes (that we could apply here)?

Yes, you can disable checks per file. This is yoda-conditions from SIM.

Would it be useful if I jump in and try? — Asking to make sure we're not both working on it at the same time.

I can do it, since I'm looking at it now. It's just a simple series of commands:

$ git checkout master -- tests/test_enum.py $ pre-commit run -a ... Fixed 19 errors: - tests/test_enum.py: 10 × SIM201 (negate-equal-op) 7 × SIM300 (yoda-conditions) 2 × SIM202 (negate-not-equal-op) $ git restore .

Then I add those codes to pyproject.toml.

Thanks for the explanation!

In pyproject.toml:

"tests/test_enum.py" = ["SIM201", "SIM300", "SIM202"]

For someone externally this will be very difficult to discover. Preferred alternative to make this more obvious (tested):

test_embed.py (e.g. top):

# It is critical that ruff does not rewrite the tests for overloaded operators: # ruff: noqa SIM201 SIM202 SIM300

A) that was just added a release or two ago :)

B) I'm not sure that's the right syntax. Did you verify that this is not a global ignore of all codes + some extra comment letters? The docs say ruff: noqa: SIM201 SIM202 SIM300 (note the second colon).

Sure, happy to do it that way.

Answer to B: yes, that's exactly what it is. If the colon is missing, that's just a comment after a total noqa. Hmm, wait a minute, even with the colon, it still seems to be ignoring too much. Will investigate.

Answer to B: yes, that's exactly what it is.

Oh ... sorry, I picked up the syntax from some documentation page and apparently was too happy too quickly. Thanks for looking into this!

It does show a warning, but you need a Ruff new enough to support it, which I didn't have. It's (as of writing) the latest version now.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

Skylion007 · 2023-02-21T22:47:03Z

pyproject.toml

+  "RET",         # flake8-return
+  "RUF100",      # Ruff-specific
+  "SIM",         # flake8-simplify
+  "UP",          # pyupgrade


Can we enable keep runtime typing for Pyupgrade?

We can disable the pyupgrade code for those per file pattern, which is about the same (better in some ways, worse in others). Won't make a difference until py37 is the minimum, I think.

Skylion007 · 2023-02-21T22:47:34Z

.pre-commit-config.yaml

-  rev: "v2.1.3"
+# Ruff, the Python auto-correcting linter written in Rust
+- repo: https://github.com/charliermarsh/ruff-pre-commit
+  rev: v0.0.247


This version is already out of date please autoupdate

You also need another arg now to provide a non-zero exit code when fixits are applied.

I don't think pre-commit cares - it can detect changed files.

If not, this flag needs to be added to the pre-commit hook under entry.

rwgk

I ran C++ coverage analysis 1. for master as-is, 2. with this PR as-is.

The results are exactly identical (line coverage & branch coverage).

I did that twice.
I also removed a couple tests, to convince myself that there are differences in that case.
I think we're good!

It would be great if we could have the test_enum.py suppressions in the file itself, but if that's not easily possible, I'm fine with this PR as-is.

Skylion007 · 2023-02-21T22:49:53Z

pyproject.toml

+]
+
+[tool.ruff]
+select = [


Newest version of ruff has PEP8-naming rules implemented. We should enable those too

pybind11/setup_helpers.py:274:7: N801 Class name `build_ext` should use CapWords convention tests/test_exceptions.py:292:7: N818 Exception name `FlakyException` should be named with an Error suffix tests/test_pytypes.py:18:5: N802 Function name `test_handle_from_move_only_type_with_operator_PyObject` should be lowercase tests/test_sequences_and_iterators.py:157:11: N818 Exception name `BadLen` should be named with an Error suffix Found 4 errors.

Not sure it really matters in tests, and the one in the project is wrong. Guess I can just use --add-noqa and inject these four skips, though.

Okay, skipped naming rules for tests, and noqa'd the only non-test one.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii · 2023-02-22T14:18:02Z

We do have them in-file now.

henryiii force-pushed the henryiii/chore/ruff branch 2 times, most recently from 7948ee8 to f98562b Compare February 1, 2023 19:37

rwgk reviewed Feb 1, 2023

View reviewed changes

henryiii force-pushed the henryiii/chore/ruff branch 2 times, most recently from 6438372 to e660fb2 Compare February 9, 2023 13:40

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11clif#4483

Closed

henryiii force-pushed the henryiii/chore/ruff branch 2 times, most recently from 5621b34 to 54e491b Compare February 16, 2023 15:55

henryiii commented Feb 17, 2023

View reviewed changes

henryiii force-pushed the henryiii/chore/ruff branch from 54e491b to d36b7d2 Compare February 17, 2023 22:03

henryiii added 2 commits February 17, 2023 17:04

chore: move to Ruff and add rules

b73a354

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

tests: drop unneeded capsys

990c22b

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii force-pushed the henryiii/chore/ruff branch from d36b7d2 to be9a604 Compare February 17, 2023 22:04

fix: fixups

2248f65

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii force-pushed the henryiii/chore/ruff branch from be9a604 to 2248f65 Compare February 17, 2023 22:21

rwgk reviewed Feb 18, 2023

View reviewed changes

rwgk reviewed Feb 21, 2023

View reviewed changes

tests: avoid changing operator order in operator tests

8028ac4

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii mentioned this pull request Feb 21, 2023

Specific ruff file ignores ignore everything astral-sh/ruff#3101

Closed

Skylion007 reviewed Feb 21, 2023

View reviewed changes

rwgk approved these changes Feb 21, 2023

View reviewed changes

Skylion007 reviewed Feb 21, 2023

View reviewed changes

chore: update Ruff version and address feedback

9c52f90

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii merged commit 438034c into pybind:master Feb 22, 2023

henryiii deleted the henryiii/chore/ruff branch February 22, 2023 14:18

github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 22, 2023

rwgk mentioned this pull request Feb 22, 2023

[smart_holder] git merge master #4524

Merged

rwgk mentioned this pull request Feb 22, 2023

[smart_holder] git merge master #4525

Merged

henryiii removed the needs changelog Possibly needs a changelog entry label Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move to Ruff and add rules #4483

chore: move to Ruff and add rules #4483

henryiii commented Feb 1, 2023 •

edited

Loading

rwgk left a comment

rwgk Feb 1, 2023

rwgk Feb 1, 2023

henryiii Feb 17, 2023

henryiii commented Feb 18, 2023

rwgk left a comment

rwgk Feb 18, 2023

Skylion007 Feb 18, 2023

henryiii Feb 21, 2023

rwgk left a comment

rwgk Feb 20, 2023

henryiii Feb 21, 2023 •

edited

Loading

rwgk Feb 20, 2023

henryiii Feb 21, 2023 •

edited

Loading

rwgk Feb 21, 2023

henryiii Feb 21, 2023 •

edited

Loading

rwgk Feb 21, 2023

henryiii Feb 21, 2023

henryiii Feb 21, 2023

rwgk Feb 21, 2023

henryiii Feb 22, 2023

Skylion007 Feb 21, 2023

henryiii Feb 22, 2023

Skylion007 Feb 21, 2023

Skylion007 Feb 21, 2023

henryiii Feb 22, 2023

henryiii Feb 22, 2023

rwgk left a comment

Skylion007 Feb 21, 2023

henryiii Feb 22, 2023

henryiii Feb 22, 2023

henryiii commented Feb 22, 2023

chore: move to Ruff and add rules #4483

chore: move to Ruff and add rules #4483

Conversation

henryiii commented Feb 1, 2023 • edited Loading

Description

Suggested changelog entry:

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Feb 18, 2023

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Feb 22, 2023

henryiii commented Feb 1, 2023 •

edited

Loading

henryiii Feb 21, 2023 •

edited

Loading

henryiii Feb 21, 2023 •

edited

Loading

henryiii Feb 21, 2023 •

edited

Loading