[flake8-use-pathlib] Fix PTH101, PTH104, PTH105, PTH121 fixes
#20143
Conversation
|
chirizxc
changed the title
flake8-use-pathlib] Fix PTH101, PTH104, PTH105, PTH121 fixesflake8-use-pathlib] Fix PTH101, PTH104, PTH105, PTH121 fixes
|
I don't know how to add a co-author to a PR, maybe only maintainers can do that? |
|
i use this cases: import os
from pathlib import Path
# os.chmod(mode=0o600, follow_symlinks=True, path="pth1_link")
os.chmod(mode=0o600, follow_symlinks=True, path="pth1_link")
Path("pth1_link").chmod(mode=0o600)
# os.chmod("pth1_link", mode=0o600, follow_symlinks= False )
os.chmod("pth1_link", mode=0o600, follow_symlinks= False )
Path("pth1_link").chmod(mode=0o600, follow_symlinks=False)
# os.chmod(path="pth1_link", mode=0o600, follow_symlinks=True)
os.chmod(path="pth1_link", mode=0o600, follow_symlinks=True)
Path("pth1_link").chmod(mode=0o600)
# os.chmod("pth1_link", follow_symlinks=True, mode=0o600)
os.chmod("pth1_link", follow_symlinks=True, mode=0o600)
Path("pth1_link").chmod(mode=0o600)
# os.chmod("pth1_link", follow_symlinks=False, mode=0o600, dir_fd=None)
os.chmod("pth1_link", follow_symlinks=False, mode=0o600, dir_fd=None)
Path("pth1_link").chmod(follow_symlinks=False, mode=0o600)
# os.chmod("pth1_link", 0o600)
os.chmod("pth1_link", 0o600)
Path("pth1_link").chmod(0o600)
# os.chmod("pth1_link", 0o600, dir_fd=None)
os.chmod("pth1_link", 0o600, dir_fd=None)
Path("pth1_link").chmod(0o600)
# os.chmod("pth1_link", 0o600, follow_symlinks=False)
os.chmod("pth1_link", 0o600, follow_symlinks=False)
Path("pth1_link").chmod(0o600, follow_symlinks=False)
# os.chmod("pth1_link", 0o600, dir_fd=None, follow_symlinks=False)
os.chmod("pth1_link", 0o600, dir_fd=None, follow_symlinks=False)
Path("pth1_link").chmod(0o600, follow_symlinks=False)
# Only diagnostic
os.chmod("pth1_link", 0o600, follow_symlinks="False")
os.chmod("pth1_file", 0o700, None, True, 1, *[1], **{"x": 1}, foo=1)
os.chmod()
os.chmod("pth1_link", follow_symlinks=True)
os.chmod("pth1_link", dir_fd=None) |
You can add a co-author in git with a special line like this in a commit message: But I can also add that when merging, no worries! I'll try to give this a review soon. |
ntBre
added
bug
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! Just a few comments/suggestions
| }) | ||
| } | ||
|
|
||
| pub(crate) fn is_optional_bool_literal(args: &Arguments, name: &str, pos: usize) -> bool { |
There was a problem hiding this comment.
It looks like we're often calling this either before or after a separate call to args.find_argument_value(name, pos). Would it make sense just to pass in the Expr from that? Alternatively, maybe this could return Some(&Expr) or Some(&ExprBooleanLiteral) instead of a bool, just to avoid calling find_argument_value multiple times.
I don't think it's too big of a deal, just an idea.
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_chmod.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_chmod.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_chmod.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_makedirs.rs
Outdated
Show resolved
Hide resolved
...e_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview_full_name.py.snap
Outdated
Show resolved
Hide resolved
|
I think this good now |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this is looking good! Two more small things (and a conflict from the other PTH PR, it looks like) and then I think this is good to go.
| Some("mode") => Some(format!("mode={}", locator.slice(&kw.value))), | ||
| Some("follow_symlinks") => { | ||
| Some(format!("follow_symlinks={}", locator.slice(&kw.value))) | ||
| } |
There was a problem hiding this comment.
Could we use the trick like in the other PR where we slice out the whole kw?
| Some("mode") => Some(format!("mode={}", locator.slice(&kw.value))), | |
| Some("follow_symlinks") => { | |
| Some(format!("follow_symlinks={}", locator.slice(&kw.value))) | |
| } | |
| Some("mode" | "follow_symlinks") => locator.slice(kw), |
then we can avoid the to_string call for positional args too.
There was a problem hiding this comment.
error[E0308]: `match` arms have incompatible types
--> crates\ruff_linter\src\rules\flake8_use_pathlib\rules\os_chmod.rs:132:22
|
130 | ArgOrKeyword::Keyword(kw) => match kw.arg.as_deref() {
| __________________________________________-
131 | | Some("mode" | "follow_symlinks") => locator.slice(kw),
| | ----------------- this is found to be of type `&str`
132 | | _ => None,
| | ^^^^ expected `&str`, found `Option<_>`
133 | | },
| |_____________- `match` arms have incompatible types
|
= note: expected reference `&str`
found enum `std::option::Option<_>
There was a problem hiding this comment.
Oh oops, I forgot to wrap it in Some, sorry! I think we can still remove to_string as long as we remove it from the ArgOrKeyword::Arg arm too.
crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_symlink.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Parizher <[email protected]>
|
I couldn't find another e-mail on github so I used |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you! I just pushed one commit adding back the PTH211 test case. It seemed fine to keep it and a bit unrelated to the other changes here, from what I could tell.
2 participants
Summary
Fixes #20134
Test Plan
cargo nextest run flake8_use_pathlib