Skip to content

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

Merged
merged 29 commits into from
May 5, 2025

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Apr 4, 2025

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 and with write that can be further rewritten to be more concise with Path.read_text and Path.write_text

@Avasam Avasam marked this pull request as draft April 4, 2025 19:10
@Avasam Avasam changed the title Enable ruff flake8 use pathlib (pth) Enable Ruff flake8-use-pathlib (PTH) Apr 4, 2025
@@ -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:
Copy link
Collaborator

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.

@Avasam Avasam marked this pull request as ready for review May 3, 2025 21:23
Comment on lines 16 to 22
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()]
Copy link
Collaborator Author

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)

@@ -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()]):
Copy link
Collaborator Author

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)

@srittau
Copy link
Collaborator

srittau commented May 5, 2025

I've gone ahead and merged #13943 for now (extracting _get_relative() can be postponed to a subsequent PR) to unblock this PR. Which now has merge conflicts, of course.

Copy link
Collaborator

@srittau srittau left a 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.

Copy link
Contributor

github-actions bot commented May 5, 2025

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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])
Copy link
Collaborator Author

@Avasam Avasam May 5, 2025

Choose a reason for hiding this comment

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

This could also be:

Suggested change
missing_modules.add(module_path.rsplit(".", 1)[0])
missing_modules.add(module_path.removesuffix(".pyi"))

which reads clearer to me

Copy link
Collaborator

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.

Copy link
Collaborator Author

@Avasam Avasam May 5, 2025

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")

Comment on lines 200 to 215
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
Copy link
Collaborator Author

@Avasam Avasam May 5, 2025

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)

Copy link
Collaborator

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.)

Copy link
Collaborator Author

@Avasam Avasam May 5, 2025

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.

@Avasam Avasam requested a review from srittau May 5, 2025 15:49
@srittau srittau merged commit 4265ee7 into python:main May 5, 2025
58 checks passed
@Avasam Avasam deleted the Enable-Ruff-flake8-use-pathlib-(PTH) branch May 5, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Applying more Ruff groups to this repository
3 participants