Skip to content

Commit

Permalink
[flake8-use-pathlib] Extend check for invalid path suffix to includ…
Browse files Browse the repository at this point in the history
…e the case `"."` (`PTH210`) (#14902)

## Summary

`PTH210` renamed to `invalid-pathlib-with-suffix` and extended to check for `.with_suffix(".")`. This caused the fix availability to be downgraded to "Sometimes", since there is no fix offered in this case.

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Dylan <53534755+dylwil3@users.noreply.github.com>
  • Loading branch information
3 people authored Dec 12, 2024
1 parent 71239f2 commit 68e8496
Show file tree
Hide file tree
Showing 9 changed files with 902 additions and 727 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,37 @@


### Errors
path.with_suffix(".")
path.with_suffix("py")
path.with_suffix(r"s")
path.with_suffix(u'' "json")
path.with_suffix(suffix="js")

posix_path.with_suffix(".")
posix_path.with_suffix("py")
posix_path.with_suffix(r"s")
posix_path.with_suffix(u'' "json")
posix_path.with_suffix(suffix="js")

pure_path.with_suffix(".")
pure_path.with_suffix("py")
pure_path.with_suffix(r"s")
pure_path.with_suffix(u'' "json")
pure_path.with_suffix(suffix="js")

pure_posix_path.with_suffix(".")
pure_posix_path.with_suffix("py")
pure_posix_path.with_suffix(r"s")
pure_posix_path.with_suffix(u'' "json")
pure_posix_path.with_suffix(suffix="js")

pure_windows_path.with_suffix(".")
pure_windows_path.with_suffix("py")
pure_windows_path.with_suffix(r"s")
pure_windows_path.with_suffix(u'' "json")
pure_windows_path.with_suffix(suffix="js")

windows_path.with_suffix(".")
windows_path.with_suffix("py")
windows_path.with_suffix(r"s")
windows_path.with_suffix(u'' "json")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

def test_path(p: Path) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand All @@ -27,6 +28,7 @@ def test_path(p: Path) -> None:

def test_posix_path(p: PosixPath) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand All @@ -44,6 +46,7 @@ def test_posix_path(p: PosixPath) -> None:

def test_pure_path(p: PurePath) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand All @@ -61,6 +64,7 @@ def test_pure_path(p: PurePath) -> None:

def test_pure_posix_path(p: PurePosixPath) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand All @@ -78,6 +82,7 @@ def test_pure_posix_path(p: PurePosixPath) -> None:

def test_pure_windows_path(p: PureWindowsPath) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand All @@ -95,6 +100,7 @@ def test_pure_windows_path(p: PureWindowsPath) -> None:

def test_windows_path(p: WindowsPath) -> None:
## Errors
p.with_suffix(".")
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1096,8 +1096,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryCastToInt) {
ruff::rules::unnecessary_cast_to_int(checker, call);
}
if checker.enabled(Rule::DotlessPathlibWithSuffix) {
flake8_use_pathlib::rules::dotless_pathlib_with_suffix(checker, call);
if checker.enabled(Rule::InvalidPathlibWithSuffix) {
flake8_use_pathlib::rules::invalid_pathlib_with_suffix(checker, call);
}
if checker.enabled(Rule::BatchedWithoutExplicitStrict) {
flake8_bugbear::rules::batched_without_explicit_strict(checker, call);
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8UsePathlib, "206") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsSepSplit),
(Flake8UsePathlib, "207") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::Glob),
(Flake8UsePathlib, "208") => (RuleGroup::Preview, rules::flake8_use_pathlib::violations::OsListdir),
(Flake8UsePathlib, "210") => (RuleGroup::Preview, rules::flake8_use_pathlib::rules::DotlessPathlibWithSuffix),
(Flake8UsePathlib, "210") => (RuleGroup::Preview, rules::flake8_use_pathlib::rules::InvalidPathlibWithSuffix),

// flake8-logging-format
(Flake8LoggingFormat, "001") => (RuleGroup::Stable, rules::flake8_logging_format::violations::LoggingStringFormat),
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ mod tests {
#[test_case(Rule::OsSepSplit, Path::new("PTH206.py"))]
#[test_case(Rule::Glob, Path::new("PTH207.py"))]
#[test_case(Rule::OsListdir, Path::new("PTH208.py"))]
#[test_case(Rule::DotlessPathlibWithSuffix, Path::new("PTH210.py"))]
#[test_case(Rule::DotlessPathlibWithSuffix, Path::new("PTH210_1.py"))]
#[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210.py"))]
#[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210_1.py"))]
fn rules_pypath(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::checkers::ast::Checker;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{self as ast, StringFlags};
use ruff_python_semantic::analyze::typing;
Expand All @@ -8,11 +8,13 @@ use ruff_text_size::Ranged;

/// ## What it does
/// Checks for `pathlib.Path.with_suffix()` calls where
/// the given suffix does not have a leading dot.
/// the given suffix does not have a leading dot
/// or the given suffix is a single dot `"."`.
///
/// ## Why is this bad?
/// `Path.with_suffix()` will raise an error at runtime
/// if the given suffix is not prefixed with a dot.
/// if the given suffix is not prefixed with a dot
/// or it is a single dot `"."`.
///
/// ## Examples
///
Expand Down Expand Up @@ -43,22 +45,40 @@ use ruff_text_size::Ranged;
/// Moreover, it's impossible to determine if this is the correct fix
/// for a given situation (it's possible that the string was correct
/// but was being passed to the wrong method entirely, for example).
///
/// No fix is offered if the suffix `"."` is given, since the intent is unclear.
#[derive(ViolationMetadata)]
pub(crate) struct DotlessPathlibWithSuffix;
pub(crate) struct InvalidPathlibWithSuffix {
// TODO: Since "." is a correct suffix in Python 3.14,
// we will need to update this rule and documentation
// once Ruff supports Python 3.14.
single_dot: bool,
}

impl Violation for InvalidPathlibWithSuffix {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

impl AlwaysFixableViolation for DotlessPathlibWithSuffix {
#[derive_message_formats]
fn message(&self) -> String {
"Dotless suffix passed to `.with_suffix()`".to_string()
if self.single_dot {
"Invalid suffix passed to `.with_suffix()`".to_string()
} else {
"Dotless suffix passed to `.with_suffix()`".to_string()
}
}

fn fix_title(&self) -> String {
"Add a leading dot".to_string()
fn fix_title(&self) -> Option<String> {
let title = if self.single_dot {
"Remove \".\" or extend to valid suffix"
} else {
"Add a leading dot"
};
Some(title.to_string())
}
}

/// PTH210
pub(crate) fn dotless_pathlib_with_suffix(checker: &mut Checker, call: &ast::ExprCall) {
pub(crate) fn invalid_pathlib_with_suffix(checker: &mut Checker, call: &ast::ExprCall) {
let (func, arguments) = (&call.func, &call.arguments);

if !is_path_with_suffix_call(checker.semantic(), func) {
Expand All @@ -75,18 +95,29 @@ pub(crate) fn dotless_pathlib_with_suffix(checker: &mut Checker, call: &ast::Exp

let string_value = string.value.to_str();

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

if string_value.starts_with('.') && string_value.len() > 1 {
return;
}

let [first_part, ..] = string.value.as_slice() else {
return;
};

let diagnostic = Diagnostic::new(DotlessPathlibWithSuffix, call.range);
let after_leading_quote = string.start() + first_part.flags.opener_len();
let fix = Fix::unsafe_edit(Edit::insertion(".".to_string(), after_leading_quote));
checker.diagnostics.push(diagnostic.with_fix(fix));
let single_dot = string_value == ".";
let mut diagnostic = Diagnostic::new(InvalidPathlibWithSuffix { single_dot }, call.range);
if !single_dot {
let after_leading_quote = string.start() + first_part.flags.opener_len();
diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
".".to_string(),
after_leading_quote,
)));
}

checker.diagnostics.push(diagnostic);
}

fn is_path_with_suffix_call(semantic: &SemanticModel, func: &ast::Expr) -> bool {
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
pub(crate) use dotless_pathlib_with_suffix::*;
pub(crate) use glob_rule::*;
pub(crate) use invalid_pathlib_with_suffix::*;
pub(crate) use os_path_getatime::*;
pub(crate) use os_path_getctime::*;
pub(crate) use os_path_getmtime::*;
Expand All @@ -8,8 +8,8 @@ pub(crate) use os_sep_split::*;
pub(crate) use path_constructor_current_directory::*;
pub(crate) use replaceable_by_pathlib::*;

mod dotless_pathlib_with_suffix;
mod glob_rule;
mod invalid_pathlib_with_suffix;
mod os_path_getatime;
mod os_path_getctime;
mod os_path_getmtime;
Expand Down
Loading

0 comments on commit 68e8496

Please sign in to comment.