[flake8-use-pathlib] Recommend Path.iterdir() over os.listdir()…

… (`PTH208`) (#14509)

Co-authored-by: Micha Reiser <[email protected]>

crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH208.py

@@ -0,0 +1,23 @@
+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)
+
+
+if os.listdir("dir"):
+    ...
+
+if "file" in os.listdir("dir"):
+    ...

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);
             }

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),

crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs

@@ -63,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(

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,
         })
     {

crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH208_PTH208.py.snap

@@ -0,0 +1,60 @@
+---
+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
+   |
+
+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 |     ...
+   |

crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs

@@ -1103,3 +1103,48 @@ 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.listdir()`.
+///
+/// ## Example
+///
+/// ```python
+/// p = "."
+/// for d in os.listdir(p):
+///     ...
+///
+/// if os.listdir(p):
+///     ...
+///
+/// if "file" in os.listdir(p):
+///     ...
+/// ```
+///
+/// Use instead:
+///
+/// ```python
+/// p = Path(".")
+/// for d in p.iterdir():
+///     ...
+///
+/// if any(p.iterdir()):
+///     ...
+///
+/// if (p / "file").exists():
+///     ...
+/// ```
+#[derive(ViolationMetadata)]
+pub(crate) struct OsListdir;
+
+impl Violation for OsListdir {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "Use `pathlib.Path.iterdir()` instead.".to_string()
+    }
+}