diff --git a/crates/ruff/resources/test/fixtures/flake8_use_pathlib/PTH206.py b/crates/ruff/resources/test/fixtures/flake8_use_pathlib/PTH206.py new file mode 100644 index 0000000000000..946d2061d000a --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_use_pathlib/PTH206.py @@ -0,0 +1,20 @@ +import os +from os import sep + + +file_name = "foo/bar" + +# PTH206 +"foo/bar/".split(os.sep) +"foo/bar/".split(sep=os.sep) +"foo/bar/".split(os.sep)[-1] +"foo/bar/".split(os.sep)[-2] +"foo/bar/".split(os.sep)[-2:] +"fizz/buzz".split(sep) +"fizz/buzz".split(sep)[-1] +os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] +file_name.split(os.sep) +(os.path.abspath(file_name)).split(os.sep) + +# OK +"foo/bar/".split("/") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index d952f21acebd8..50c411a8b9632 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3037,6 +3037,9 @@ where if self.enabled(Rule::PathConstructorCurrentDirectory) { flake8_use_pathlib::rules::path_constructor_current_directory(self, expr, func); } + if self.enabled(Rule::OsSepSplit) { + flake8_use_pathlib::rules::os_sep_split(self, func, args, keywords); + } if self.enabled(Rule::NumpyLegacyRandom) { numpy::rules::legacy_random(self, func); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 7ca269945b568..51d447ccd89f6 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -756,6 +756,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8UsePathlib, "203") => (RuleGroup::Unspecified, rules::flake8_use_pathlib::rules::OsPathGetatime), (Flake8UsePathlib, "204") => (RuleGroup::Unspecified, rules::flake8_use_pathlib::rules::OsPathGetmtime), (Flake8UsePathlib, "205") => (RuleGroup::Unspecified, rules::flake8_use_pathlib::rules::OsPathGetctime), + (Flake8UsePathlib, "206") => (RuleGroup::Unspecified, rules::flake8_use_pathlib::rules::OsSepSplit), // flake8-logging-format (Flake8LoggingFormat, "001") => (RuleGroup::Unspecified, rules::flake8_logging_format::violations::LoggingStringFormat), diff --git a/crates/ruff/src/rules/flake8_use_pathlib/mod.rs b/crates/ruff/src/rules/flake8_use_pathlib/mod.rs index 49787956292a4..5ba8d96a363e6 100644 --- a/crates/ruff/src/rules/flake8_use_pathlib/mod.rs +++ b/crates/ruff/src/rules/flake8_use_pathlib/mod.rs @@ -61,6 +61,7 @@ mod tests { #[test_case(Rule::OsPathGetatime, Path::new("PTH203.py"))] #[test_case(Rule::OsPathGetmtime, Path::new("PTH204.py"))] #[test_case(Rule::OsPathGetctime, Path::new("PTH205.py"))] + #[test_case(Rule::OsSepSplit, Path::new("PTH206.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( diff --git a/crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs b/crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs index 07966ab00acbb..9147bbaeed2db 100644 --- a/crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs @@ -2,6 +2,7 @@ pub(crate) use os_path_getatime::*; pub(crate) use os_path_getctime::*; pub(crate) use os_path_getmtime::*; pub(crate) use os_path_getsize::*; +pub(crate) use os_sep_split::*; pub(crate) use path_constructor_current_directory::*; pub(crate) use replaceable_by_pathlib::*; @@ -9,5 +10,6 @@ mod os_path_getatime; mod os_path_getctime; mod os_path_getmtime; mod os_path_getsize; +mod os_sep_split; mod path_constructor_current_directory; mod replaceable_by_pathlib; diff --git a/crates/ruff/src/rules/flake8_use_pathlib/rules/os_sep_split.rs b/crates/ruff/src/rules/flake8_use_pathlib/rules/os_sep_split.rs new file mode 100644 index 0000000000000..dfff78b9ee9df --- /dev/null +++ b/crates/ruff/src/rules/flake8_use_pathlib/rules/os_sep_split.rs @@ -0,0 +1,98 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::find_keyword; +use rustpython_parser::ast::{Expr, ExprAttribute, Keyword, Ranged}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of `.split(os.sep)` +/// +/// ## Why is this bad? +/// The `pathlib` module in the standard library should be used for path +/// manipulation. It provides a high-level API with the functionality +/// needed for common operations on `Path` objects. +/// +/// ## Example +/// If not all parts of the path are needed, then the `name` and `parent` +/// attributes of the `Path` object should be used. Otherwise, the `parts` +/// attribute can be used as shown in the last example. +/// ```python +/// import os +/// +/// "path/to/file_name.txt".split(os.sep)[-1] +/// +/// "path/to/file_name.txt".split(os.sep)[-2] +/// +/// # Iterating over the path parts +/// if any(part in blocklist for part in "my/file/path".split(os.sep)): +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// from pathlib import Path +/// +/// Path("path/to/file_name.txt").name +/// +/// Path("path/to/file_name.txt").parent.name +/// +/// # Iterating over the path parts +/// if any(part in blocklist for part in Path("my/file/path").parts): +/// ... +/// ``` +#[violation] +pub struct OsSepSplit; + +impl Violation for OsSepSplit { + #[derive_message_formats] + fn message(&self) -> String { + format!("Replace `.split(os.sep)` with `Path.parts`") + } +} + +/// PTH206 +pub(crate) fn os_sep_split( + checker: &mut Checker, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { + let Expr::Attribute(ExprAttribute { attr, .. }) = func else { + return; + }; + + if attr.as_str() != "split" { + return; + }; + + let sep = if !args.is_empty() { + // `.split(os.sep)` + let [arg] = args else { + return; + }; + arg + } else if !keywords.is_empty() { + // `.split(sep=os.sep)` + let Some(keyword) = find_keyword(keywords, "sep") else { + return; + }; + &keyword.value + } else { + return; + }; + + if !checker + .semantic() + .resolve_call_path(sep) + .map_or(false, |call_path| { + matches!(call_path.as_slice(), ["os", "sep"]) + }) + { + return; + } + + checker + .diagnostics + .push(Diagnostic::new(OsSepSplit, attr.range())); +} diff --git a/crates/ruff/src/rules/flake8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH206_PTH206.py.snap b/crates/ruff/src/rules/flake8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH206_PTH206.py.snap new file mode 100644 index 0000000000000..cd41c2038a359 --- /dev/null +++ b/crates/ruff/src/rules/flake8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH206_PTH206.py.snap @@ -0,0 +1,102 @@ +--- +source: crates/ruff/src/rules/flake8_use_pathlib/mod.rs +--- +PTH206.py:8:12: PTH206 Replace `.split(os.sep)` with `Path.parts` + | + 7 | # PTH206 + 8 | "foo/bar/".split(os.sep) + | ^^^^^ PTH206 + 9 | "foo/bar/".split(sep=os.sep) +10 | "foo/bar/".split(os.sep)[-1] + | + +PTH206.py:9:12: PTH206 Replace `.split(os.sep)` with `Path.parts` + | + 7 | # PTH206 + 8 | "foo/bar/".split(os.sep) + 9 | "foo/bar/".split(sep=os.sep) + | ^^^^^ PTH206 +10 | "foo/bar/".split(os.sep)[-1] +11 | "foo/bar/".split(os.sep)[-2] + | + +PTH206.py:10:12: PTH206 Replace `.split(os.sep)` with `Path.parts` + | + 8 | "foo/bar/".split(os.sep) + 9 | "foo/bar/".split(sep=os.sep) +10 | "foo/bar/".split(os.sep)[-1] + | ^^^^^ PTH206 +11 | "foo/bar/".split(os.sep)[-2] +12 | "foo/bar/".split(os.sep)[-2:] + | + +PTH206.py:11:12: PTH206 Replace `.split(os.sep)` with `Path.parts` + | + 9 | "foo/bar/".split(sep=os.sep) +10 | "foo/bar/".split(os.sep)[-1] +11 | "foo/bar/".split(os.sep)[-2] + | ^^^^^ PTH206 +12 | "foo/bar/".split(os.sep)[-2:] +13 | "fizz/buzz".split(sep) + | + +PTH206.py:12:12: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +10 | "foo/bar/".split(os.sep)[-1] +11 | "foo/bar/".split(os.sep)[-2] +12 | "foo/bar/".split(os.sep)[-2:] + | ^^^^^ PTH206 +13 | "fizz/buzz".split(sep) +14 | "fizz/buzz".split(sep)[-1] + | + +PTH206.py:13:13: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +11 | "foo/bar/".split(os.sep)[-2] +12 | "foo/bar/".split(os.sep)[-2:] +13 | "fizz/buzz".split(sep) + | ^^^^^ PTH206 +14 | "fizz/buzz".split(sep)[-1] +15 | os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] + | + +PTH206.py:14:13: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +12 | "foo/bar/".split(os.sep)[-2:] +13 | "fizz/buzz".split(sep) +14 | "fizz/buzz".split(sep)[-1] + | ^^^^^ PTH206 +15 | os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] +16 | file_name.split(os.sep) + | + +PTH206.py:15:47: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +13 | "fizz/buzz".split(sep) +14 | "fizz/buzz".split(sep)[-1] +15 | os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] + | ^^^^^ PTH206 +16 | file_name.split(os.sep) +17 | (os.path.abspath(file_name)).split(os.sep) + | + +PTH206.py:16:11: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +14 | "fizz/buzz".split(sep)[-1] +15 | os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] +16 | file_name.split(os.sep) + | ^^^^^ PTH206 +17 | (os.path.abspath(file_name)).split(os.sep) + | + +PTH206.py:17:30: PTH206 Replace `.split(os.sep)` with `Path.parts` + | +15 | os.path.splitext("path/to/hello_world.py")[0].split(os.sep)[-1] +16 | file_name.split(os.sep) +17 | (os.path.abspath(file_name)).split(os.sep) + | ^^^^^ PTH206 +18 | +19 | # OK + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 20530484bf28e..722af19b89d1d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2333,6 +2333,7 @@ "PTH203", "PTH204", "PTH205", + "PTH206", "PYI", "PYI0", "PYI00",