-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from all commits
b73a354
990c22b
2248f65
8028ac4
9c52f90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -58,4 +52,44 @@ messages_control.disable = [ | |
"invalid-name", | ||
"protected-access", | ||
"missing-module-docstring", | ||
"unused-argument", # covered by Ruff ARG | ||
] | ||
|
||
[tool.ruff] | ||
select = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure it really matters in tests, and the one in the project is wrong. Guess I can just use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we enable keep runtime typing for Pyupgrade? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
|
@@ -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: | ||
|
@@ -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 | ||
|
@@ -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) | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to use
as a way to document what is (obviously) intentionally unused, rather than using suppressions. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"""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(): | ||
|
@@ -220,7 +204,7 @@ def gc_collect(): | |
|
||
|
||
def pytest_configure(): | ||
pytest.suppress = suppress | ||
pytest.suppress = contextlib.suppress | ||
pytest.gc_collect = gc_collect | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://beta.ruff.rs/docs/
There was a problem hiding this comment.
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.