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

Enable Ruff SIM #13309

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
7 changes: 5 additions & 2 deletions pyproject.toml
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,13 @@ 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()`
# contextlib.suppress is roughly 3x slower than try/except
"SIM105", # flake8-simplify: use-contextlib-suppress
# 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
15 changes: 4 additions & 11 deletions scripts/stubsabot.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,7 @@ 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(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 Expand Up @@ -747,10 +744,7 @@ async def main() -> None:
parser.add_argument("distributions", nargs="*", help="Distributions to update, default = all")
args = parser.parse_args()

if args.distributions:
dists_to_update = args.distributions
else:
dists_to_update = [path.name for path in STUBS_PATH.iterdir()]
dists_to_update = args.distributions if args.distributions else [path.name for path in STUBS_PATH.iterdir()]

if args.action_level > ActionLevel.nothing:
subprocess.run(["git", "update-index", "--refresh"], capture_output=True)
Expand All @@ -765,9 +759,8 @@ async def main() -> None:
print(f"Cannot run stubsabot, as uncommitted changes are present in {changed_files}!")
sys.exit(1)

if args.action_level > ActionLevel.fork:
if os.environ.get("GITHUB_TOKEN") is None:
raise ValueError("GITHUB_TOKEN environment variable must be set")
if args.action_level > ActionLevel.fork and os.environ.get("GITHUB_TOKEN") is None:
raise ValueError("GITHUB_TOKEN environment variable must be set")

denylist = {"gdb"} # gdb is not a pypi distribution

Expand Down
4 changes: 1 addition & 3 deletions stdlib/_ssl.pyi
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.

OP_IGNORE_UNEXPECTED_EOF: int
if sys.version_info >= (3, 12):
OP_LEGACY_SERVER_CONNECT: int
Expand Down
5 changes: 1 addition & 4 deletions tests/pytype_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,7 @@ def run_all_tests(*, files_to_test: Sequence[str], print_stderr: bool, dry_run:
python_version = f"{sys.version_info.major}.{sys.version_info.minor}"
print("Testing files with pytype...")
for i, f in enumerate(files_to_test):
if dry_run:
stderr = None
else:
stderr = run_pytype(filename=f, python_version=python_version, missing_modules=missing_modules)
stderr = None if dry_run else run_pytype(filename=f, python_version=python_version, missing_modules=missing_modules)
if stderr:
if print_stderr:
print(f"\n{stderr}")
Expand Down
15 changes: 5 additions & 10 deletions tests/stubtest_third_party.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,11 @@ def run_stubtest(
stubtest_env = os.environ | {"MYPYPATH": mypypath, "MYPY_FORCE_COLOR": "1"}

# Perform some black magic in order to run stubtest inside uWSGI
if dist_name == "uWSGI":
if not setup_uwsgi_stubtest_command(dist, venv_dir, stubtest_cmd):
return False
if dist_name == "uWSGI" and not setup_uwsgi_stubtest_command(dist, venv_dir, stubtest_cmd):
return False

if dist_name == "gdb":
if not setup_gdb_stubtest_command(venv_dir, stubtest_cmd):
return False
if dist_name == "gdb" and not setup_gdb_stubtest_command(venv_dir, stubtest_cmd):
return False

try:
subprocess.run(stubtest_cmd, env=stubtest_env, check=True, capture_output=True)
Expand Down Expand Up @@ -390,10 +388,7 @@ def main() -> NoReturn:
parser.add_argument("dists", metavar="DISTRIBUTION", type=str, nargs=argparse.ZERO_OR_MORE)
args = parser.parse_args()

if len(args.dists) == 0:
dists = sorted(STUBS_PATH.iterdir())
else:
dists = [STUBS_PATH / d for d in args.dists]
dists = sorted(STUBS_PATH.iterdir()) if len(args.dists) == 0 else [STUBS_PATH / d for d in args.dists]

result = 0
for i, dist in enumerate(dists):
Expand Down
Loading