-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[flake8-use-pathlib
] Add fixes for PTH102
and PTH103
#19514
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
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PTH103 | 200 | 0 | 0 | 200 | 0 |
PTH102 | 12 | 0 | 0 | 12 | 0 |
I see I made a mistake somewhere |
Yes, it's because of a condition in |
now autofixes will be for all possible cases except |
I think this PR can also include |
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! I have a few suggestions here, but most of them are pretty minor, and this looks good overall. The main question is around adding the parents=True
argument.
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_mkdir.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_mkdir.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_mkdir.rs
Outdated
Show resolved
Hide resolved
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! This looks good except for some more concerns about argument handling, sorry for not catching this earlier.
in this variant only for the case when all arguments are positional fix will make them named to avoid the problem with |
Maybe there are more cases that I didn't see, I'll try to see more |
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
We have only two autofix outputs, when all args positional:
os.makedirs("name", 0o777, False) # =>
pathlib.Path("name").mkdir(mode=0o777, exist_ok=False, parents=True) os.makedirs("name", 0o777, False) # =>
pathlib.Path("name").mkdir(0o777, True, False) |
…inal call uses exactly 3 positional arguments and no keywords
# ruff: noqa: FBT003
import os
os.makedirs("name", True)
os.makedirs("name", 0o777)
os.makedirs("name", 0o777, True)
os.makedirs("name", 0o777, exist_ok=True)
os.makedirs("name", mode=0o777, exist_ok=True)
os.makedirs(name="name", mode=0o777, exist_ok=True)
os.makedirs("name", exist_ok=True, mode=0o777)
os.makedirs("name", idk="123")
os.makedirs("name", mode=0o777, exist_ok=False, x=True) after fix: # ruff: noqa: FBT003
import os
from pathlib import Path
Path("name").mkdir(True, parents=True)
Path("name").mkdir(0o777, parents=True)
Path("name").mkdir(0o777, True, True)
Path("name").mkdir(0o777, exist_ok=True, parents=True)
Path("name").mkdir(mode=0o777, exist_ok=True, parents=True)
Path("name").mkdir(mode=0o777, exist_ok=True, parents=True)
Path("name").mkdir(exist_ok=True, mode=0o777, parents=True)
os.makedirs("name", idk="123")
os.makedirs("name", mode=0o777, exist_ok=False, x=True) |
@ntBre can you check this realization please? |
Yes, sorry, this has been on my todo list. Do you have a question about the implementation or is this just ready for review again? And sorry for all the snapshot conflicts, we just updated our diagnostic rendering. I think you should just need to accept the new versions of the snapshots after re-running the tests. |
it's ready |
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.
Thank you, this is looking good. I had a couple more, smaller argument validation suggestions and a potential simplification for the parents
argument addition. Thanks for handling all of my pedantic feedback here, these are going to be great fixes :)
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
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.
Thank you!
flake8-use-pathlib
] Add autofix for PTH102
, PTH103
flake8-use-pathlib
] Add fixes for PTH102
and PTH103
* main: (29 commits) [ty] add docstrings to completions based on type (#20008) [`pyupgrade`] Avoid reporting `__future__` features as unnecessary when they are used (`UP010`) (#19769) [`flake8-use-pathlib`] Add fixes for `PTH102` and `PTH103` (#19514) [ty] correctly ignore field specifiers when not specified (#20002) `Option::unwrap` is now const (#20007) [ty] Re-arrange "list modules" implementation for Salsa caching [ty] Test "list modules" versus "resolve module" in every mdtest [ty] Wire up "list modules" API to make module completions work [ty] Tweak some completion tests [ty] Add "list modules" implementation [ty] Lightly expose `FileModule` and `NamespacePackage` fields [ty] Add some more helper routines to `ModulePath` [ty] Fix a bug when converting `ModulePath` to `ModuleName` [ty] Split out another constructor for `ModuleName` [ty] Add stub-file tests to existing module resolver [ty] Expose some routines in the module resolver [ty] Add more path helper functions [`flake8-annotations`] Remove unused import in example (`ANN401`) (#20000) [ty] distinguish base conda from child conda (#19990) [ty] Fix server hang (#19991) ...
Summary
Part of #2331
Test Plan
cargo nextest run flake8_use_pathlib