-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Enable Ruff flake8-use-pathlib (PTH) #13795
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 flake8-use-pathlib (PTH) #13795
Conversation
…-Ruff-flake8-use-pathlib-(PTH)
lib/ts_utils/utils.py
Outdated
@@ -203,7 +203,7 @@ def allowlists(distribution_name: str) -> list[str]: | |||
|
|||
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -472,7 +472,7 @@ def test_third_party_stubs(args: TestConfig, tempdir: Path) -> TestSummary: | |||
gitignore_spec = get_gitignore_spec() | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, to me as well.
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.
Oh, haha, I know why this breaks. The suffix is .pyi\n
😅
I went with .strip()
to keep the readability of .removesuffix(".pyi")
tests/pytype_test.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 read
andwith write
that can be further rewritten to be more concise withPath.read_text
andPath.write_text