Uh oh!
There was an error while loading. Please reload this page.
Enable Ruff flake8-use-pathlib (PTH) - #13795
Conversation
…-Ruff-flake8-use-pathlib-(PTH)
| @functools.cache | ||
| def get_gitignore_spec() -> pathspec.PathSpec: | ||
| with open(".gitignore", encoding="UTF-8") as f: | ||
| with Path(".gitignore").open(encoding="UTF-8") as f: |
There was a problem hiding this comment.
I prefer the simple open() in this and similar cases. But it's no deal breaker for me. The rest looks good.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
….com/Avasam/typeshed into Enable-Ruff-flake8-use-pathlib-(PTH)
…-Ruff-flake8-use-pathlib-(PTH)
| distributions = os.listdir(STUBS_PATH) | ||
| distributions = [distribution.name for distribution in STUBS_PATH.iterdir()] | ||
| return set(itertools.chain.from_iterable([read_dependencies(distribution).external_pkgs for distribution in distributions])) | ||
| def get_stubtest_system_requirements(distributions: Iterable[str] = (), platform: str = sys.platform) -> list[str]: | ||
| if not distributions: | ||
| distributions = os.listdir(STUBS_PATH) | ||
| distributions = [distribution.name for distribution in STUBS_PATH.iterdir()] |
There was a problem hiding this comment.
This seems like a straight up downgrade.
os-listdir (PTH208)
Only one other change of os.listdir --> Path.iterdir seems like an actual improvement. (in tests/mypy_test.py)
| distributions_to_check: dict[str, PackageDependencies] = {} | ||
| for distribution in sorted(os.listdir("stubs")): | ||
| for distribution in sorted([distribution.name for distribution in STUBS_PATH.iterdir()]): |
There was a problem hiding this comment.
This seems like a straight up downgrade.
os-listdir (PTH208)
Only one other change of os.listdir --> Path.iterdir seems like an actual improvement. (in add_third_party_files above)
srittau
commented
May 5, 2025
I've gone ahead and merged #13943 for now (extracting |
srittau
left a comment
There was a problem hiding this comment.
Thanks, just two comments below.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
….com/Avasam/typeshed into Enable-Ruff-flake8-use-pathlib-(PTH)
…-Ruff-flake8-use-pathlib-(PTH)
| unused_stubs_prefix, unused_pkg, mod_path = fi.split("/", 2) # pyright: ignore[reportUnusedVariable] | ||
| missing_modules.add(os.path.splitext(mod_path)[0]) | ||
| _ts_subdir, _distribution, module_path = line.split("/", 2) | ||
| missing_modules.add(module_path.rsplit(".", 1)[0]) |
There was a problem hiding this comment.
This could also be:
| missing_modules.add(module_path.rsplit(".", 1)[0]) | |
| missing_modules.add(module_path.removesuffix(".pyi")) |
which reads clearer to me
There was a problem hiding this comment.
Oh, haha, I know why this breaks. The suffix is .pyi\n 😅
I went with .strip() to keep the readability of .removesuffix(".pyi")
| missing_modules = { | ||
| associated_package | ||
| for distribution in stub_distributions | ||
| for external_req in read_dependencies(distribution).external_pkgs | ||
| for associated_package in _get_pkgs_associated_with_requirement(external_req.name) | ||
| } | ||
| with EXCLUDE_LIST.open() as f: | ||
| for line in f: | ||
| if not line.startswith("stubs/"): | ||
| # Skips comments, empty lines, and stdlib files, which are in | ||
| # the exclude list because pytype has its own version. | ||
| continue | ||
| unused_stubs_prefix, unused_pkg, mod_path = fi.split("/", 2) # pyright: ignore[reportUnusedVariable] | ||
| missing_modules.add(os.path.splitext(mod_path)[0]) | ||
| _ts_subdir, _distribution, module_path = line.split("/", 2) | ||
| missing_modules.add(module_path.rsplit(".", 1)[0]) | ||
| return missing_modules |
There was a problem hiding this comment.
FWIW, this could also be:
return {
associated_packagefordistributioninstub_distributionsforexternal_reqinread_dependencies(distribution).external_pkgsforassociated_packagein_get_pkgs_associated_with_requirement(external_req.name)
} | {
# Exclude the distribution and file extension from pathline.split("/", 2)[2].removesuffix(".pyi")
forlineinEXCLUDE_LIST.read_text().splitlines()
# Skips comments, empty lines, and stdlib files, which are in# the exclude list because pytype has its own version.ifline.startswith("stubs/")
}(not sure for line in EXCLUDE_LIST.read_text().splitlines() is really much better than with EXCLUDE_LIST.open() as f: for line in f:, it's was more as a personal exercise to spot where comprehensions could be used. And could doesn't mean should XD)
There was a problem hiding this comment.
I wouldn't want to do fairly extensive changes like this in a PR that's mostly about introducing Path. (Also, the code above is a bit too clever for my taste.)
There was a problem hiding this comment.
Yeah agreed. I don't wanna push this code-change.
Speaking of changes not directly related to introducing Path (or rather, enforcing it). There's still a few changes in the pytype script that were more a result of looking at it more closely. I could split those off as well if you prefer.
Port existing code to pathlib
Kept this one for last. Closes#13295
Some rules in this group are forcing some refactoring to use
pathlib. In some cases, it's a lot cleaner, in others it's debatable.I unconditionally followed all the rules. Please indicate changes you disagree with/preferred in the old style. I'll revert those and disable the relevant rules.
Edit: os-listdir (PTH208) was disabled, see #13795 (comment) and #13795 (comment)
There's also a few
with readandwith writethat can be further rewritten to be more concise withPath.read_textandPath.write_text