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

[flake8-use-pathlib] Extend check for invalid path suffix to include the case "." (PTH210) #14902

Merged
merged 14 commits into from
Dec 12, 2024

Conversation

TheBits
Copy link
Contributor

@TheBits TheBits commented Dec 10, 2024

Summary

Hi!

Thank you @InSyncWithFoo for the your contribution #14779!

I suggest adding a test that makes use of the suffix='.'. It must fail. Right now, ruff doesn't show any errors in this test.

Test Plan

Tests fails

@dylwil3 dylwil3 added bug Something isn't working rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 10, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 10, 2024

Thanks for suggesting this test and pointing out the bug! Of course, since it fails we can't merge it in without also having the fix 😄 Would you be interested in taking a look and fleshing out your PR? I believe this early return needs to be modified:

if string_value.is_empty() || string_value.starts_with('.') {
return;
}

Edit: Actually the change will be a little more involved because I don't think it makes sense to offer a fix in this case, so you'll have to change the rule to be only sometimes fixable. Let me know if you're interested and/or need any pointers navigating that change.

@InSyncWithFoo
Copy link
Contributor

The rule, which is currently called dotless-pathlib-with-suffix, might also need to be renamed. Its documentation requires an update as well.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@TheBits
Copy link
Contributor Author

TheBits commented Dec 11, 2024

@dylwil3 Yes, I need help. I tried to figure it out and fix the dotless_pathlib_with_suffix. Have I made the appropriate corrections?

@MichaReiser
Copy link
Member

The rule, which is currently called _dotless_-pathlib-with-suffix, might also need to be renamed. Its documentation requires an update as well.

Agree, we should probably rename it to invalid-pathlib-with-suffix or similar.

Copy link
Collaborator

@dylwil3 dylwil3 left a comment

Choose a reason for hiding this comment

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

Looking good! In addition to Micha's suggestions, could you update the documentation comment?

@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 11, 2024

We may want to add a TODO comment for when Python 3.14 is released/supported by Ruff. The rule should revert to the original behavior in that case since "." is a valid suffix in 3.14.

@TheBits
Copy link
Contributor Author

TheBits commented Dec 11, 2024

Looking good! In addition to Micha's suggestions, could you update the documentation comment?

Thanks! I will try to update documentation comment.

Copy link
Collaborator

@dylwil3 dylwil3 left a comment

Choose a reason for hiding this comment

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

Few more nits - almost there!

@MichaReiser MichaReiser requested a review from dylwil3 December 12, 2024 13:35
Copy link
Collaborator

@dylwil3 dylwil3 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much for your patience, and for the great contribution!

@dylwil3 dylwil3 changed the title [flake8-use-pathlib] The Path.with_suffix('.') test should fail (PTH210) [flake8-use-pathlib] Extend check for invalid path suffix to include the case "." (PTH210) Dec 12, 2024
@dylwil3 dylwil3 merged commit 68e8496 into astral-sh:main Dec 12, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants