-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[flake8-use-pathlib] Mark fixes unsafe for return type changes (PTH104, PTH105, PTH109, PTH115)
#21440
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] Mark fixes unsafe for return type changes (PTH104, PTH105, PTH109, PTH115)
#21440
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,12 +94,22 @@ pub(crate) fn check_os_pathlib_single_arg_calls( | |
| let fix = match applicability { | ||
| Some(Applicability::Unsafe) => Fix::unsafe_edits(edit, [import_edit]), | ||
| _ => { | ||
| let applicability = if checker.comment_ranges().intersects(range) { | ||
| let determined_applicability = if checker.comment_ranges().intersects(range) { | ||
| Applicability::Unsafe | ||
| } else if applicability.is_none() { | ||
| // When applicability is None, we need to determine it based on return type changes | ||
| if is_top_level_expression_call(checker, call) { | ||
| // Safe when the call is a top-level expression (return value not used) | ||
| Applicability::Safe | ||
| } else { | ||
| // Unsafe because the return type changes (str/bytes -> Path or None -> Path) | ||
| Applicability::Unsafe | ||
| } | ||
| } else { | ||
| // applicability is Some(Applicability::Safe), use it | ||
| Applicability::Safe | ||
| }; | ||
| Fix::applicable_edits(edit, [import_edit], applicability) | ||
| Fix::applicable_edits(edit, [import_edit], determined_applicability) | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -176,8 +186,12 @@ pub(crate) fn check_os_pathlib_two_arg_calls( | |
|
|
||
| let applicability = if checker.comment_ranges().intersects(range) { | ||
| Applicability::Unsafe | ||
| } else { | ||
| } else if is_top_level_expression_call(checker, call) { | ||
| // Safe when the call is a top-level expression (return value not used) | ||
| Applicability::Safe | ||
| } else { | ||
| // Unsafe because the return type changes (None -> Path) | ||
| Applicability::Unsafe | ||
|
||
| }; | ||
|
|
||
| Ok(Fix::applicable_edits( | ||
|
|
@@ -209,3 +223,9 @@ pub(crate) fn is_argument_non_default(arguments: &Arguments, name: &str, positio | |
| .find_argument_value(name, position) | ||
| .is_some_and(|expr| !expr.is_none_literal_expr()) | ||
| } | ||
|
|
||
| /// Returns `true` if the given call is a top-level expression in its statement. | ||
| /// This means the call's return value is not used, so return type changes don't matter. | ||
| pub(crate) fn is_top_level_expression_call(checker: &Checker, _call: &ExprCall) -> bool { | ||
ntBre marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| checker.semantic().current_expression_parent().is_none() | ||
| } | ||
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 don't think we should do this check here. I checked all of the references for
check_os_pathlib_single_arg_calls, and it's only used by PTH115, of the rules we're trying to fix in this PR.I think we should just do the
is_top_level_expression_callcheck in PTH115 itself.