Enable Ruff flake8-use-pathlib (PTH)#13795
Conversation
…-Ruff-flake8-use-pathlib-(PTH)
lib/ts_utils/utils.py
Outdated
| @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.
….com/Avasam/typeshed into Enable-Ruff-flake8-use-pathlib-(PTH)
…-Ruff-flake8-use-pathlib-(PTH)
lib/ts_utils/requirements.py
Outdated
| 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)
tests/mypy_test.py
Outdated
| 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)
|
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.
|
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)
tests/pytype_test.py
Outdated
| 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_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)
} | {
# Exclude the distribution and file extension from path
line.split("/", 2)[2].removesuffix(".pyi")
for line in EXCLUDE_LIST.read_text().splitlines()
# Skips comments, empty lines, and stdlib files, which are in
# the exclude list because pytype has its own version.
if line.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