From 92e6957962e688101a743fef9456f9f4dd0fdd27 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Thu, 21 Nov 2024 10:29:27 +0000 Subject: [PATCH 1/5] [`flake8-use-pathlib`] Recommend `Path.iterdir()` over `os.listdir()` (`PTH208`) --- .../fixtures/flake8_use_pathlib/PTH208.py | 16 +++++++ .../src/checkers/ast/analyze/expression.rs | 1 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_use_pathlib/mod.rs | 19 ++++++++ .../rules/replaceable_by_pathlib.rs | 10 +++-- ...lib__tests__preview__PTH208_PTH208.py.snap | 44 +++++++++++++++++++ .../rules/flake8_use_pathlib/violations.rs | 33 ++++++++++++++ 7 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py create mode 100644 crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py new file mode 100644 index 0000000000000..23f3abd63dd24 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py @@ -0,0 +1,16 @@ +import os + +os.listdir('.') +os.listdir(b'.') + +string_path = '.' +os.listdir(string_path) + +bytes_path = b'.' +os.listdir(bytes_path) + + +from pathlib import Path + +path_path = Path('.') +os.listdir(path_path) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 0c2f4e235c087..6ca710fd8f17b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -965,6 +965,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { Rule::OsPathGetmtime, Rule::OsPathGetctime, Rule::Glob, + Rule::OsListdir, ]) { flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 1ce84c6a2b63d..d4a9ec964139b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -908,6 +908,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8UsePathlib, "205") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsPathGetctime), (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), // flake8-logging-format (Flake8LoggingFormat, "001") => (RuleGroup::Stable, rules::flake8_logging_format::violations::LoggingStringFormat), diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs index 01b1ae356800e..619ba731a387d 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs @@ -12,6 +12,7 @@ mod tests { use crate::assert_messages; use crate::registry::Rule; use crate::settings; + use crate::settings::types::PreviewMode; use crate::test::test_path; #[test_case(Path::new("full_name.py"))] @@ -72,4 +73,22 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::OsListdir, Path::new("PTH208.py"))] + fn preview_rules_pth(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("flake8_use_pathlib").join(path).as_path(), + &settings::LinterSettings { + preview: PreviewMode::Enabled, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs index aa1db51279f61..fd97c1fe3b8ac 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs @@ -10,10 +10,10 @@ use crate::rules::flake8_use_pathlib::rules::{ Glob, OsPathGetatime, OsPathGetctime, OsPathGetmtime, OsPathGetsize, }; use crate::rules::flake8_use_pathlib::violations::{ - BuiltinOpen, Joiner, OsChmod, OsGetcwd, OsMakedirs, OsMkdir, OsPathAbspath, OsPathBasename, - OsPathDirname, OsPathExists, OsPathExpanduser, OsPathIsabs, OsPathIsdir, OsPathIsfile, - OsPathIslink, OsPathJoin, OsPathSamefile, OsPathSplitext, OsReadlink, OsRemove, OsRename, - OsReplace, OsRmdir, OsStat, OsUnlink, PyPath, + BuiltinOpen, Joiner, OsChmod, OsGetcwd, OsListdir, OsMakedirs, OsMkdir, OsPathAbspath, + OsPathBasename, OsPathDirname, OsPathExists, OsPathExpanduser, OsPathIsabs, OsPathIsdir, + OsPathIsfile, OsPathIslink, OsPathJoin, OsPathSamefile, OsPathSplitext, OsReadlink, OsRemove, + OsRename, OsReplace, OsRmdir, OsStat, OsUnlink, PyPath, }; use crate::settings::types::PythonVersion; @@ -155,6 +155,8 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, call: &ExprCall) { ["os", "readlink"] if checker.settings.target_version >= PythonVersion::Py39 => { Some(OsReadlink.into()) } + // PTH208, + ["os", "listdir"] => Some(OsListdir.into()), _ => None, }) { diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap new file mode 100644 index 0000000000000..4797d9c3f567e --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap @@ -0,0 +1,44 @@ +--- +source: crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs +snapshot_kind: text +--- +PTH208.py:3:1: PTH208 Use `pathlib.Path.iterdir()` instead. + | +1 | import os +2 | +3 | os.listdir('.') + | ^^^^^^^^^^ PTH208 +4 | os.listdir(b'.') + | + +PTH208.py:4:1: PTH208 Use `pathlib.Path.iterdir()` instead. + | +3 | os.listdir('.') +4 | os.listdir(b'.') + | ^^^^^^^^^^ PTH208 +5 | +6 | string_path = '.' + | + +PTH208.py:7:1: PTH208 Use `pathlib.Path.iterdir()` instead. + | +6 | string_path = '.' +7 | os.listdir(string_path) + | ^^^^^^^^^^ PTH208 +8 | +9 | bytes_path = b'.' + | + +PTH208.py:10:1: PTH208 Use `pathlib.Path.iterdir()` instead. + | + 9 | bytes_path = b'.' +10 | os.listdir(bytes_path) + | ^^^^^^^^^^ PTH208 + | + +PTH208.py:16:1: PTH208 Use `pathlib.Path.iterdir()` instead. + | +15 | path_path = Path('.') +16 | os.listdir(path_path) + | ^^^^^^^^^^ PTH208 + | diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index 01e88ea89b85c..c0f07bf5e750e 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -1103,3 +1103,36 @@ impl Violation for PyPath { "`py.path` is in maintenance mode, use `pathlib` instead".to_string() } } + +/// ## What it does +/// Checks for uses of `os.listdir`. +/// +/// ## Why is this bad? +/// `pathlib` offers a high-level API for path manipulation, as compared to +/// the lower-level API offered by `os`. When possible, using `pathlib`'s +/// `Path.iterdir()` can improve readability over `os`'s `listdir()`. +/// +/// ## Example +/// +/// ```python +/// p = "." +/// for d in os.listdir(p)): +/// ... +/// ``` +/// +/// Use instead: +/// +/// ```python +/// p = Path(".") +/// for d in p.iterdir(): +/// ... +/// ``` +#[violation] +pub struct OsListdir; + +impl Violation for OsListdir { + #[derive_message_formats] + fn message(&self) -> String { + "Use `pathlib.Path.iterdir()` instead.".to_string() + } +} From 677266965afe1eb94896ca10411d1202759fd767 Mon Sep 17 00:00:00 2001 From: InSync Date: Thu, 21 Nov 2024 17:37:23 +0700 Subject: [PATCH 2/5] Schema --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index fee16aaec8c57..2ba04db4e103a 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3711,6 +3711,7 @@ "PTH205", "PTH206", "PTH207", + "PTH208", "PYI", "PYI0", "PYI00", From 972c3a748d211519be2265e94b8ba5945431ef4e Mon Sep 17 00:00:00 2001 From: InSync Date: Thu, 21 Nov 2024 17:47:39 +0700 Subject: [PATCH 3/5] Extraneous closing parenthesis --- crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index c0f07bf5e750e..aada6b77dffa7 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -1116,7 +1116,7 @@ impl Violation for PyPath { /// /// ```python /// p = "." -/// for d in os.listdir(p)): +/// for d in os.listdir(p): /// ... /// ``` /// From b3dc296709115e62692c472bec9371bb21ddf6d1 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Wed, 27 Nov 2024 09:25:33 +0000 Subject: [PATCH 4/5] Per review --- .../test/fixtures/flake8_use_pathlib/PTH208.py | 7 +++++++ ...athlib__tests__preview__PTH208_PTH208.py.snap | 16 ++++++++++++++++ .../src/rules/flake8_use_pathlib/violations.rs | 12 ++++++++++++ 3 files changed, 35 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py index 23f3abd63dd24..8a31b2b472e05 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py @@ -14,3 +14,10 @@ path_path = Path('.') os.listdir(path_path) + + +if os.listdir("dir"): + ... + +if "file" in os.listdir("dir"): + ... diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap index 4797d9c3f567e..27d5009df016d 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap @@ -42,3 +42,19 @@ PTH208.py:16:1: PTH208 Use `pathlib.Path.iterdir()` instead. 16 | os.listdir(path_path) | ^^^^^^^^^^ PTH208 | + +PTH208.py:19:4: PTH208 Use `pathlib.Path.iterdir()` instead. + | +19 | if os.listdir("dir"): + | ^^^^^^^^^^ PTH208 +20 | ... + | + +PTH208.py:22:14: PTH208 Use `pathlib.Path.iterdir()` instead. + | +20 | ... +21 | +22 | if "file" in os.listdir("dir"): + | ^^^^^^^^^^ PTH208 +23 | ... + | diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index aada6b77dffa7..e9862bfa2488a 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -1118,6 +1118,12 @@ impl Violation for PyPath { /// p = "." /// for d in os.listdir(p): /// ... +/// +/// if os.listdir(p): +/// ... +/// +/// if "file" in os.listdir(p): +/// ... /// ``` /// /// Use instead: @@ -1126,6 +1132,12 @@ impl Violation for PyPath { /// p = Path(".") /// for d in p.iterdir(): /// ... +/// +/// if any(p.iterdir()): +/// ... +/// +/// if (p / "file").exists(): +/// ... /// ``` #[violation] pub struct OsListdir; From 01f5b7fa9c1b78450a864c5ab6c722015c19bb0f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 27 Nov 2024 10:43:43 +0100 Subject: [PATCH 5/5] Remove preview-only test group --- .../src/rules/flake8_use_pathlib/mod.rs | 20 +------------------ ...use_pathlib__tests__PTH208_PTH208.py.snap} | 0 .../rules/flake8_use_pathlib/violations.rs | 6 +++--- 3 files changed, 4 insertions(+), 22 deletions(-) rename crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/{ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap => ruff_linter__rules__flake8_use_pathlib__tests__PTH208_PTH208.py.snap} (100%) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs index 619ba731a387d..5b678c8823e8c 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs @@ -12,7 +12,6 @@ mod tests { use crate::assert_messages; use crate::registry::Rule; use crate::settings; - use crate::settings::types::PreviewMode; use crate::test::test_path; #[test_case(Path::new("full_name.py"))] @@ -64,6 +63,7 @@ mod tests { #[test_case(Rule::OsPathGetctime, Path::new("PTH205.py"))] #[test_case(Rule::OsSepSplit, Path::new("PTH206.py"))] #[test_case(Rule::Glob, Path::new("PTH207.py"))] + #[test_case(Rule::OsListdir, Path::new("PTH208.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( @@ -73,22 +73,4 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } - - #[test_case(Rule::OsListdir, Path::new("PTH208.py"))] - fn preview_rules_pth(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!( - "preview__{}_{}", - rule_code.noqa_code(), - path.to_string_lossy() - ); - let diagnostics = test_path( - Path::new("flake8_use_pathlib").join(path).as_path(), - &settings::LinterSettings { - preview: PreviewMode::Enabled, - ..settings::LinterSettings::for_rule(rule_code) - }, - )?; - assert_messages!(snapshot, diagnostics); - Ok(()) - } } diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH208_PTH208.py.snap similarity index 100% rename from crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview__PTH208_PTH208.py.snap rename to crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH208_PTH208.py.snap diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index e9862bfa2488a..12dd0044765b6 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -1110,7 +1110,7 @@ impl Violation for PyPath { /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to /// the lower-level API offered by `os`. When possible, using `pathlib`'s -/// `Path.iterdir()` can improve readability over `os`'s `listdir()`. +/// `Path.iterdir()` can improve readability over `os.listdir()`. /// /// ## Example /// @@ -1139,8 +1139,8 @@ impl Violation for PyPath { /// if (p / "file").exists(): /// ... /// ``` -#[violation] -pub struct OsListdir; +#[derive(ViolationMetadata)] +pub(crate) struct OsListdir; impl Violation for OsListdir { #[derive_message_formats]