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.

Sign up for GitHub

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

Enable Ruff SIM #13309

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Enable Ruff SIM #13309

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eee5ab1
Enable Ruff SIM
Avasam Dec 26, 2024
a96249e
Work around PYI002+SIM114 incompatibility
Avasam Dec 26, 2024
1daf81f
Disable SIM102 and SIM108
Avasam Dec 26, 2024
6126a6a
Merge branch 'main' of https://github.com/python/typeshed into Ruff-SIM
Avasam Dec 29, 2024
7d5868e
Disable SIM110
Avasam Dec 29, 2024
93d24f8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 29, 2024
570c84c
Merge branch 'main' into Ruff-SIM
Avasam Jan 2, 2025
892301b
Apply suggestions from code review
Avasam Jan 2, 2025
818c0d3
More specific SIM selection
Avasam Jan 6, 2025
b7e39ea
Merge branch 'main' of https://github.com/python/typeshed into Ruff-SIM
Avasam Jan 6, 2025
b40aa9b
Expand SIM2
Avasam Jan 6, 2025
c52feda
typo
Avasam Jan 6, 2025
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
10 changes: 8 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ select = [
"FA", # flake8-future-annotations
"I", # isort
"RUF", # Ruff-specific and unused-noqa
"SIM", # flake8-simplify
Copy link
Member

Choose a reason for hiding this comment

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

I still think I'd rather explicitly list the rules we think are useful here, rather than enabling the whole category and disabling specific rules. This category is just too hit-and-miss for me: some of the rules are pretty clear wins, but quite a few make some very opinionated suggestions that have high false-positive rates. Even if some of those rules don't have any false positives on typeshed today, there's a chance we might add code in the future that they'd emit false positives on, or there's a chance that Ruff might stabilise new SIM rules that have false positives on typeshed code.

If lint rules end up irritating us (or confusing contributors) more than they actually help us write better code, then it defeats the purpose of a linter, in my opinion :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to be more specific about rules selection
I listed the rules I didn't select in the PR's description

"UP", # pyupgrade
# Flake8 base rules
"E", # pycodestyle Error
Expand Down Expand Up @@ -86,11 +87,16 @@ ignore = [
###
# Rules we don't want or don't agree with
###
# Slower and more verbose https://github.com/astral-sh/ruff/issues/7871
"UP038", # Use `X | Y` in `isinstance` call instead of `(X, Y)`
# Used for direct, non-subclass type comparison, for example: `type(val) is str`
# see https://github.com/astral-sh/ruff/issues/6465
"E721", # Do not compare types, use `isinstance()`
# Long single-line ternaries tend to be less readable with our 130 line-length
"SIM102", # Use a single if statement instead of nested if statements
"SIM108", # Use ternary operator {contents} instead of if-else-block
# contextlib.suppress is roughly 3x slower than try/except
"SIM105", # Use contextlib.suppress() instead of try-except-pass
# Slower and more verbose https://github.com/astral-sh/ruff/issues/7871
"UP038", # Use `X | Y` in `isinstance` call instead of `(X, Y)`
###
# False-positives, but already checked by type-checkers
###
Expand Down
9 changes: 5 additions & 4 deletions scripts/stubsabot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,11 @@ def all_py_files_in_source_are_in_py_typed_dirs(source: zipfile.ZipFile | tarfil
if not all_python_files:
return False

for path in all_python_files:
if not any(py_typed_dir in path.parents for py_typed_dir in py_typed_dirs):
return False
return True
return all(
# fmt: skip
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? This looks like how Black would format the code anyway.

Copy link
Collaborator Author

@Avasam Avasam Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With our 130 line length black puts the whole thing on a single line. Which greatly affected readability. (Black and Ruff don't tend to have great vertical formatting for ternaries and comprehensions when the line length is long).

For the sake of discussion about the "reimplemented-builtin (SIM110)" rule, I worked around this formatting issue

Copy link
Member

Choose a reason for hiding this comment

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

If you need to manually format the result, that seems like an argument against applying this "simplification" rule.

any(py_typed_dir in path.parents for py_typed_dir in py_typed_dirs)
for path in all_python_files
)


async def release_contains_py_typed(release_to_download: PypiReleaseDownload, *, session: aiohttp.ClientSession) -> bool:
Expand Down
4 changes: 1 addition & 3 deletions stdlib/_ssl.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,7 @@ OP_SINGLE_ECDH_USE: int
OP_NO_COMPRESSION: int
OP_ENABLE_MIDDLEBOX_COMPAT: int
OP_NO_RENEGOTIATION: int
if sys.version_info >= (3, 11):
OP_IGNORE_UNEXPECTED_EOF: int
elif sys.version_info >= (3, 8) and sys.platform == "linux":
if sys.version_info >= (3, 11) or sys.platform == "linux":
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

astral-sh/ruff#15153

OP_IGNORE_UNEXPECTED_EOF: int
if sys.version_info >= (3, 12):
OP_LEGACY_SERVER_CONNECT: int
Expand Down
Loading