Skip to content

Commit

Permalink
[flake8-use-pathlib] Implement os-sep-split (PTH206) (#5936)
Browse files Browse the repository at this point in the history
Implements
#5905 (comment)

---------

Co-authored-by: konsti <konstin@mailbox.org>
  • Loading branch information
sbrugman and konstin authored Jul 23, 2023
1 parent 057faab commit f886b58
Show file tree
Hide file tree
Showing 8 changed files with 228 additions and 0 deletions.
20 changes: 20 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_use_pathlib/PTH206.py
Original file line number Diff line number Diff line change
@@ -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("/")
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ 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::*;

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;
98 changes: 98 additions & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/rules/os_sep_split.rs
Original file line number Diff line number Diff line change
@@ -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()));
}
Original file line number Diff line number Diff line change
@@ -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
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit f886b58

Please sign in to comment.