-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[pylint
] Detect indirect pathlib.Path
usages for unspecified-encoding
(PLW1514)
#19304
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 |
---|---|---|---|---|---|
PLW1514 | 206 | 206 | 0 | 0 | 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.
I think we can reuse some existing type inference code for this.
return Some(Callee::Pathlib(attr)); | ||
} | ||
// Indirect: x.open() where x = Path(...) | ||
else if let Expr::Name(name) = value.as_ref() { |
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 think this would be a good use of our typing
module:
ruff/crates/ruff_python_semantic/src/analyze/typing.rs
Lines 1089 to 1093 in 966fd6f
/// Test whether the given binding can be considered a `pathlib.PurePath` | |
/// or an instance of a subclass thereof. | |
pub fn is_pathlib_path(binding: &Binding, semantic: &SemanticModel) -> bool { | |
check_type::<PathlibPathChecker>(binding, semantic) | |
} |
It looks like we already have one for Path
. I think this will also allow us to cover the annotated function argument case from the issue, whereas I don't think this implementation would.
def format_file(file: Path):
with file.open() as f:
contents = f.read()
It might be good to add this as a test case if it does work.
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.
Looks good, just one suggestion to delete a redundant section of code.
crates/ruff_linter/src/rules/pylint/rules/unspecified_encoding.rs
Outdated
Show resolved
Hide resolved
I spot-checked the ecosystem changes, and this looks like a really handy change! We can use that to make sure my suggestion doesn't break anything too (currently +206 violations). |
Co-authored-by: Brent Westbrook <[email protected]>
Looks good to go! 🙂 |
Excellent, thank you! |
Summary
Fixes #19294