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

chore: move to Ruff and add rules #4483

Merged
merged 5 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 6 additions & 38 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,6 @@ repos:
- id: requirements-txt-fixer
- id: trailing-whitespace

# Upgrade old Python syntax
- repo: https://github.com/asottile/pyupgrade
rev: "v3.3.1"
hooks:
- id: pyupgrade
args: [--py36-plus]

# Nicely sort includes
- repo: https://github.com/PyCQA/isort
rev: "5.12.0"
hooks:
- id: isort

# Black, the code formatter, natively supports pre-commit
- repo: https://github.com/psf/black
rev: "23.1.0" # Keep in sync with blacken-docs
Expand All @@ -72,47 +59,28 @@ repos:
hooks:
- id: remove-tabs

# Avoid directional quotes
- repo: https://github.com/sirosen/texthooks
rev: "0.5.0"
hooks:
- id: fix-ligatures
- id: fix-smartquotes

# Autoremoves unused imports
- repo: https://github.com/hadialqattan/pycln
rev: "v2.1.3"
# Ruff, the Python auto-correcting linter written in Rust
- repo: https://github.com/charliermarsh/ruff-pre-commit
rev: v0.0.251
hooks:
- id: pycln
stages: [manual]
- id: ruff
args: ["--fix", "--show-fixes"]

# Checking for common mistakes
- repo: https://github.com/pre-commit/pygrep-hooks
rev: "v1.10.0"
hooks:
- id: python-check-blanket-noqa
- id: python-check-blanket-type-ignore
- id: python-no-log-warn
- id: python-use-type-annotations
- id: rst-backticks
- id: rst-directive-colons
- id: rst-inline-touching-normal

# Automatically remove noqa that are not used
- repo: https://github.com/asottile/yesqa
rev: "v1.4.0"
hooks:
- id: yesqa
additional_dependencies: &flake8_dependencies
- flake8-bugbear
- pep8-naming

# Flake8 also supports pre-commit natively (same author)
- repo: https://github.com/PyCQA/flake8
rev: "6.0.0"
hooks:
- id: flake8
exclude: ^(docs/.*|tools/.*)$
additional_dependencies: *flake8_dependencies

# PyLint has native support - not always usable, but works for us
- repo: https://github.com/PyCQA/pylint
Expand Down
2 changes: 1 addition & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ def prepare(app):
f.write(contents)


def clean_up(app, exception):
def clean_up(app, exception): # noqa: ARG001
(DIR / "readme.rst").unlink()


Expand Down
2 changes: 1 addition & 1 deletion pybind11/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import sys

if sys.version_info < (3, 6):
if sys.version_info < (3, 6): # noqa: UP036
msg = "pybind11 does not support Python < 3.6. 2.9 was the last release supporting Python 2.7 and 3.5."
raise ImportError(msg)

Expand Down
2 changes: 1 addition & 1 deletion pybind11/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
DIR = os.path.abspath(os.path.dirname(__file__))


def get_include(user: bool = False) -> str: # pylint: disable=unused-argument
def get_include(user: bool = False) -> str: # noqa: ARG001
"""
Return the path to the pybind11 include directory. The historical "user"
argument is unused, and may be removed.
Expand Down
2 changes: 1 addition & 1 deletion pybind11/setup_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ def naive_recompile(obj: str, src: str) -> bool:
return os.stat(obj).st_mtime < os.stat(src).st_mtime


def no_recompile(obg: str, src: str) -> bool: # pylint: disable=unused-argument
def no_recompile(obg: str, src: str) -> bool: # noqa: ARG001
"""
This is the safest but slowest choice (and is the default) - will always
recompile sources.
Expand Down
46 changes: 40 additions & 6 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ ignore = [
"noxfile.py",
]

[tool.isort]
# Needs the compiled .so modules and env.py from tests
known_first_party = "env,pybind11_cross_module_tests,pybind11_tests,"
# For black compatibility
profile = "black"

[tool.mypy]
files = ["pybind11"]
python_version = "3.6"
Expand Down Expand Up @@ -58,4 +52,44 @@ messages_control.disable = [
"invalid-name",
"protected-access",
"missing-module-docstring",
"unused-argument", # covered by Ruff ARG
]

[tool.ruff]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

select = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"E", "F", "W", # flake8
"B", "B904", # flake8-bugbear
"I", # isort
"N", # pep8-naming
"ARG", # flake8-unused-arguments
"C4", # flake8-comprehensions
"EM", # flake8-errmsg
"ICN", # flake8-import-conventions
"ISC", # flake8-implicit-str-concat
"PGH", # pygrep-hooks
"PIE", # flake8-pie
"PL", # pylint
"PT", # flake8-pytest-style
"RET", # flake8-return
"RUF100", # Ruff-specific
"SIM", # flake8-simplify
"UP", # pyupgrade
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enable keep runtime typing for Pyupgrade?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"YTT", # flake8-2020
]
ignore = [
"PLR", # Design related pylint
"E501", # Line too long (Black is enough)
"PT011", # Too broad with raises in pytest
"PT004", # Fixture that doesn't return needs underscore (no, it is fine)
"SIM118",# iter(x) is not always the same as iter(x.keys())
]
target-version = "py37"
typing-modules = ["scikit_build_core._compat.typing"]
src = ["src"]
unfixable = ["T20"]
exclude = []
line-length = 120
isort.known-first-party = ["env", "pybind11_cross_module_tests", "pybind11_tests"]

[tool.ruff.per-file-ignores]
"tests/**" = ["EM", "N"]
8 changes: 0 additions & 8 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,3 @@ project_urls =
[options]
python_requires = >=3.6
zip_safe = False


[flake8]
max-line-length = 120
show_source = True
exclude = .git, __pycache__, build, dist, docs, tools, venv
extend-ignore = E203, E722
extend-select = B902, B904
50 changes: 17 additions & 33 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@ def __eq__(self, other):
b = _strip_and_dedent(other).splitlines()
if a == b:
return True
else:
self.explanation = _make_explanation(a, b)
return False
self.explanation = _make_explanation(a, b)
return False


class Unordered(Output):
Expand All @@ -96,9 +95,8 @@ def __eq__(self, other):
b = _split_and_sort(other)
if a == b:
return True
else:
self.explanation = _make_explanation(a, b)
return False
self.explanation = _make_explanation(a, b)
return False


class Capture:
Expand All @@ -119,9 +117,8 @@ def __eq__(self, other):
b = other
if a == b:
return True
else:
self.explanation = a.explanation
return False
self.explanation = a.explanation
return False

def __str__(self):
return self.out
Expand All @@ -138,7 +135,7 @@ def stderr(self):
return Output(self.err)


@pytest.fixture
@pytest.fixture()
def capture(capsys):
"""Extended `capsys` with context manager and custom equality operators"""
return Capture(capsys)
Expand All @@ -159,25 +156,22 @@ def __eq__(self, other):
b = _strip_and_dedent(other)
if a == b:
return True
else:
self.explanation = _make_explanation(a.splitlines(), b.splitlines())
return False
self.explanation = _make_explanation(a.splitlines(), b.splitlines())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

return False


def _sanitize_general(s):
s = s.strip()
s = s.replace("pybind11_tests.", "m.")
s = _long_marker.sub(r"\1", s)
return s
return _long_marker.sub(r"\1", s)


def _sanitize_docstring(thing):
s = thing.__doc__
s = _sanitize_general(s)
return s
return _sanitize_general(s)
henryiii marked this conversation as resolved.
Show resolved Hide resolved


@pytest.fixture
@pytest.fixture()
def doc():
"""Sanitize docstrings and add custom failure explanation"""
return SanitizedString(_sanitize_docstring)
Expand All @@ -186,30 +180,20 @@ def doc():
def _sanitize_message(thing):
s = str(thing)
s = _sanitize_general(s)
s = _hexadecimal.sub("0", s)
return s
return _hexadecimal.sub("0", s)


@pytest.fixture
@pytest.fixture()
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use

del op
del right

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

Copy link
Collaborator Author

@henryiii henryiii Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Hook to insert custom failure explanation"""
if hasattr(left, "explanation"):
return left.explanation


@contextlib.contextmanager
def suppress(exception):
"""Suppress the desired exception"""
try:
yield
except exception:
pass
return None


def gc_collect():
Expand All @@ -220,7 +204,7 @@ def gc_collect():


def pytest_configure():
pytest.suppress = suppress
pytest.suppress = contextlib.suppress
pytest.gc_collect = gc_collect


Expand Down
3 changes: 1 addition & 2 deletions tests/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,4 @@ def deprecated_call():
pytest_major_minor = (int(pieces[0]), int(pieces[1]))
if pytest_major_minor < (3, 9):
return pytest.warns((DeprecationWarning, PendingDeprecationWarning))
else:
return pytest.deprecated_call()
return pytest.deprecated_call()
4 changes: 2 additions & 2 deletions tests/test_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
m = pytest.importorskip("pybind11_tests.async_module")


@pytest.fixture
@pytest.fixture()
def event_loop():
loop = asyncio.new_event_loop()
yield loop
Expand All @@ -16,7 +16,7 @@ async def get_await_result(x):


def test_await(event_loop):
assert 5 == event_loop.run_until_complete(get_await_result(m.SupportsAsync()))
assert event_loop.run_until_complete(get_await_result(m.SupportsAsync())) == 5


def test_await_missing(event_loop):
Expand Down
3 changes: 2 additions & 1 deletion tests/test_buffers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

assert mat2[3, 2] == 7
mat2[2, 3] = 5
assert mat2[2, 3] == 5

Expand Down
10 changes: 6 additions & 4 deletions tests/test_builtin_casters.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ def test_bytes_to_string():

assert m.strlen(b"hi") == 2
assert m.string_length(b"world") == 5
assert m.string_length("a\x00b".encode()) == 3
assert m.strlen("a\x00b".encode()) == 1 # C-string limitation
assert m.string_length(b"a\x00b") == 3
assert m.strlen(b"a\x00b") == 1 # C-string limitation

# passing in a utf8 encoded string should work
assert m.string_length("💩".encode()) == 4
Expand Down Expand Up @@ -421,13 +421,15 @@ def test_reference_wrapper():
a2 = m.refwrap_list(copy=True)
assert [x.value for x in a1] == [2, 3]
assert [x.value for x in a2] == [2, 3]
assert not a1[0] is a2[0] and not a1[1] is a2[1]
assert a1[0] is not a2[0]
assert a1[1] is not a2[1]

b1 = m.refwrap_list(copy=False)
b2 = m.refwrap_list(copy=False)
assert [x.value for x in b1] == [1, 2]
assert [x.value for x in b2] == [1, 2]
assert b1[0] is b2[0] and b1[1] is b2[1]
assert b1[0] is b2[0]
assert b1[1] is b2[1]

assert m.refwrap_iiw(IncType(5)) == 5
assert m.refwrap_call_iiw(IncType(10), m.refwrap_iiw) == [10, 10, 10, 10]
Expand Down
Loading