progress

astral-sh · Feb 11, 2023 · bdc347e · bdc347e
1 parent 69884b3
commit bdc347e
Show file tree

Hide file tree

Showing 19 changed files with 975 additions and 323 deletions.
diff --git a/README.md b/README.md
@@ -1281,28 +1281,28 @@ For more, see [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
 
 | Code | Name | Message | Fix |
 | ---- | ---- | ------- | --- |
-| PTH100 | pathlib-abspath | `os.path.abspath` should be replaced by `.resolve()` |  |
-| PTH101 | pathlib-chmod | `os.chmod` should be replaced by `.chmod()` |  |
-| PTH102 | pathlib-mkdir | `os.mkdir` should be replaced by `.mkdir()` |  |
+| PTH100 | pathlib-abspath | `os.path.abspath` should be replaced by `.resolve()` | 🛠 |
+| PTH101 | pathlib-chmod | `os.chmod` should be replaced by `.chmod()` | 🛠 |
+| PTH102 | pathlib-mkdir | `os.mkdir` should be replaced by `.mkdir()` | 🛠 |
 | PTH103 | pathlib-makedirs | `os.makedirs` should be replaced by `.mkdir(parents=True)` |  |
-| PTH104 | pathlib-rename | `os.rename` should be replaced by `.rename()` |  |
-| PTH105 | pathlib-replace | `os.replace`should be replaced by `.replace()` |  |
-| PTH106 | pathlib-rmdir | `os.rmdir` should be replaced by `.rmdir()` |  |
-| PTH107 | pathlib-remove | `os.remove` should be replaced by `.unlink()` |  |
-| PTH108 | pathlib-unlink | `os.unlink` should be replaced by `.unlink()` |  |
+| PTH104 | pathlib-rename | `os.rename` should be replaced by `.rename()` | 🛠 |
+| PTH105 | pathlib-replace | `os.replace`should be replaced by `.replace()` | 🛠 |
+| PTH106 | pathlib-rmdir | `os.rmdir` should be replaced by `.rmdir()` | 🛠 |
+| PTH107 | pathlib-remove | `os.remove` should be replaced by `.unlink()` | 🛠 |
+| PTH108 | pathlib-unlink | `os.unlink` should be replaced by `.unlink()` | 🛠 |
 | PTH109 | [pathlib-getcwd](https://github.com/charliermarsh/ruff/blob/main/docs/rules/pathlib-getcwd.md) | `os.getcwd` should be replaced by `Path.cwd()` | 🛠 |
-| PTH110 | pathlib-exists | `os.path.exists` should be replaced by `.exists()` |  |
-| PTH111 | pathlib-expanduser | `os.path.expanduser` should be replaced by `.expanduser()` |  |
-| PTH112 | pathlib-is-dir | `os.path.isdir` should be replaced by `.is_dir()` |  |
-| PTH113 | pathlib-is-file | `os.path.isfile` should be replaced by `.is_file()` |  |
-| PTH114 | pathlib-is-link | `os.path.islink` should be replaced by `.is_symlink()` |  |
-| PTH115 | pathlib-readlink | `os.readlink` should be replaced by `.readlink()` |  |
+| PTH110 | pathlib-exists | `os.path.exists` should be replaced by `.exists()` | 🛠 |
+| PTH111 | pathlib-expanduser | `os.path.expanduser` should be replaced by `.expanduser()` | 🛠 |
+| PTH112 | pathlib-is-dir | `os.path.isdir` should be replaced by `.is_dir()` | 🛠 |
+| PTH113 | pathlib-is-file | `os.path.isfile` should be replaced by `.is_file()` | 🛠 |
+| PTH114 | pathlib-is-link | `os.path.islink` should be replaced by `.is_symlink()` | 🛠 |
+| PTH115 | [pathlib-readlink](https://github.com/charliermarsh/ruff/blob/main/docs/rules/pathlib-readlink.md) | `os.readlink` should be replaced by `.readlink()` | 🛠 |
 | PTH116 | pathlib-stat | `os.stat` should be replaced by `.stat()` or `.owner()` or `.group()` |  |
-| PTH117 | pathlib-is-abs | `os.path.isabs` should be replaced by `.is_absolute()` |  |
+| PTH117 | pathlib-is-abs | `os.path.isabs` should be replaced by `.is_absolute()` | 🛠 |
 | PTH118 | pathlib-join | `os.path.join` should be replaced by `foo_path / "bar"` |  |
 | PTH119 | pathlib-basename | `os.path.basename` should be replaced by `.name` |  |
 | PTH120 | pathlib-dirname | `os.path.dirname` should be replaced by `.parent` |  |
-| PTH121 | pathlib-samefile | `os.path.samefile` should be replaced by `.samefile()` |  |
+| PTH121 | pathlib-samefile | `os.path.samefile` should be replaced by `.samefile()` | 🛠 |
 | PTH122 | pathlib-splitext | `os.path.splitext` should be replaced by `.suffix` |  |
 | PTH123 | pathlib-open | `open("foo")` should be replaced by `Path("foo").open()` |  |
 | PTH124 | pathlib-py-path | `py.path` is in maintenance mode, use `pathlib` instead |  |

diff --git a/crates/ruff/resources/test/fixtures/flake8_use_pathlib/full_name.py b/crates/ruff/resources/test/fixtures/flake8_use_pathlib/full_name.py
@@ -5,15 +5,15 @@
 p = "/foo"
 
 a = os.path.abspath(p)
-aa = os.chmod(p)
+aa = os.chmod(p, mode=511)
 aaa = os.mkdir(p)
 os.makedirs(p)
-os.rename(p)
-os.replace(p)
+os.rename(p, q)
+os.replace(p, q)
 os.rmdir(p)
 os.remove(p)
 os.unlink(p)
-os.getcwd(p)
+os.getcwd()
 b = os.path.exists(p)
 bb = os.path.expanduser(p)
 bbb = os.path.isdir(p)
@@ -30,4 +30,4 @@
 with open(p) as fp:
     fp.read()
 open(p).close()
-os.getcwdb(p)
+os.getcwdb()
diff --git a/crates/ruff/resources/test/fixtures/flake8_use_pathlib/use_pathlib.py b/crates/ruff/resources/test/fixtures/flake8_use_pathlib/use_pathlib.py
@@ -16,3 +16,6 @@
 
 # should not be unwrapped
 _ = Path(os.getcwd(), hello='world')
+
+# other cases
+os.path.abspath("../../hello.py")
diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs
@@ -2758,6 +2758,7 @@ where
                 {
                     flake8_use_pathlib::helpers::replaceable_by_pathlib(
                         self,
+                        expr,
                         func,
                         self.current_expr_parent().map(Into::into),
                     );

diff --git a/crates/ruff/src/rules/flake8_use_pathlib/fixes.rs b/crates/ruff/src/rules/flake8_use_pathlib/fixes.rs
@@ -1,6 +1,6 @@
-use rustpython_parser::ast::{Expr, ExprKind};
+use rustpython_parser::ast::{Expr, ExprContext, ExprKind};
 
-use crate::ast::helpers;
+use crate::ast::helpers::{self, create_expr, unparse_expr};
 use crate::ast::types::Range;
 use crate::autofix::apply_fix;
 use crate::checkers::ast::Checker;
@@ -11,56 +11,111 @@ use crate::source_code::Locator;
 pub fn pathlib_fix(
     checker: &mut Checker,
     diagnostic: &DiagnosticKind,
+    expr: &Expr,
     func: &Expr,
     parent: Option<&Expr>,
 ) -> Option<Fix> {
     // Guard that Path is imported, `content` contains the name or aliaas
     if let Some(content) = helpers::get_member_import_name_alias(checker, "pathlib", "Path") {
-        let mut fix = match diagnostic {
-            DiagnosticKind::PathlibGetcwd(_) => Some(Fix::replacement(
-                format!("{content}.cwd"),
-                func.location,
-                func.end_location.unwrap(),
-            )),
-            _ => None,
-        };
+        if let ExprKind::Call { args, keywords, .. } = &expr.node {
+            // TODO: validate args/keywords, possibly map
+            // TODO: add non-call replacements
+            let replacement = match diagnostic {
+                DiagnosticKind::PathlibAbspath(_) => "resolve",
+                DiagnosticKind::PathlibChmod(_) => "chmod",
+                DiagnosticKind::PathlibMkdir(_) => "mkdir",
+                // Makedirs
+                DiagnosticKind::PathlibRename(_) => "rename",
+                DiagnosticKind::PathlibReplace(_) => "replace",
+                DiagnosticKind::PathlibRmdir(_) => "rmdir",
+                DiagnosticKind::PathlibRemove(_) => "unlink",
+                DiagnosticKind::PathlibUnlink(_) => "unlink",
+                DiagnosticKind::PathlibGetcwd(_) => "cwd",
+                DiagnosticKind::PathlibExists(_) => "exists",
+                DiagnosticKind::PathlibExpanduser(_) => "expanduser",
+                DiagnosticKind::PathlibIsDir(_) => "is_dir",
+                DiagnosticKind::PathlibIsFile(_) => "is_file",
+                DiagnosticKind::PathlibIsLink(_) => "is_symlink",
+                DiagnosticKind::PathlibReadlink(_) => "readlink",
+                // Stat
+                DiagnosticKind::PathlibIsAbs(_) => "is_absolute",
+                // Join
+                // Basename
+                // Dirname
+                DiagnosticKind::PathlibSamefile(_) => "samefile",
+                // Splitext
+                // Open
+                _ => return None,
+            };
 
-        // Wrapped in a `Path()` call
-        if let Some(fixme) = fix.clone() {
-            if let Some(parent) = parent {
-                if checker
-                    .resolve_call_path(parent)
-                    .map_or(false, |call_path| {
-                        call_path.as_slice() == ["pathlib", "Path"]
-                    })
-                {
-                    if let ExprKind::Call { args, keywords, .. } = &parent.node {
-                        if args.len() == 1 && keywords.is_empty() {
-                            // Reset the line index
-                            let fixme = Fix::replacement(
-                                fixme.content.to_string(),
-                                helpers::to_relative(fixme.location, func.location),
-                                helpers::to_relative(fixme.end_location, func.location),
-                            );
+            if let Some((head, tail)) = args.clone().split_first() {
+                let fix_str = unparse_expr(
+                    &create_expr(ExprKind::Call {
+                        func: Box::new(create_expr(ExprKind::Attribute {
+                            value: Box::new(create_expr(ExprKind::Call {
+                                func: Box::new(create_expr(ExprKind::Name {
+                                    id: content,
+                                    ctx: ExprContext::Load,
+                                })),
+                                args: vec![head.clone()],
+                                keywords: vec![],
+                            })),
+                            attr: replacement.to_string(),
+                            ctx: ExprContext::Load,
+                        })),
+                        args: tail.to_vec(),
+                        keywords: keywords.clone(),
+                    }),
+                    checker.stylist,
+                );
 
-                            // Apply the fix
-                            let arg = args.first().unwrap();
-                            let contents = checker.locator.slice_source_code_range(&Range::new(
-                                arg.location,
-                                arg.end_location.unwrap(),
-                            ));
+                let mut fix = Some(Fix::replacement(
+                    fix_str,
+                    expr.location,
+                    expr.end_location.unwrap(),
+                ));
 
-                            fix = Some(Fix::replacement(
-                                apply_fix(&fixme, &Locator::new(contents)),
-                                parent.location,
-                                parent.end_location.unwrap(),
-                            ));
+                // Wrapped in a `Path()` call
+                if let Some(fixme) = fix.clone() {
+                    if let Some(parent) = parent {
+                        if checker
+                            .resolve_call_path(parent)
+                            .map_or(false, |call_path| {
+                                call_path.as_slice() == ["pathlib", "Path"]
+                            })
+                        {
+                            if let ExprKind::Call { args, keywords, .. } = &parent.node {
+                                if args.len() == 1 && keywords.is_empty() {
+                                    // Reset the line index
+                                    let fixme = Fix::replacement(
+                                        fixme.content.to_string(),
+                                        helpers::to_relative(fixme.location, func.location),
+                                        helpers::to_relative(fixme.end_location, func.location),
+                                    );
+
+                                    // Apply the fix
+                                    let arg = args.first().unwrap();
+                                    let contents = checker.locator.slice_source_code_range(
+                                        &Range::new(arg.location, arg.end_location.unwrap()),
+                                    );
+
+                                    fix = Some(Fix::replacement(
+                                        apply_fix(&fixme, &Locator::new(contents)),
+                                        parent.location,
+                                        parent.end_location.unwrap(),
+                                    ));
+                                }
+                            }
                         }
                     }
                 }
+                fix
+            } else {
+                None
             }
+        } else {
+            None
         }
-        fix
     } else {
         None
     }

diff --git a/crates/ruff/src/rules/flake8_use_pathlib/helpers.rs b/crates/ruff/src/rules/flake8_use_pathlib/helpers.rs
@@ -14,10 +14,15 @@ use crate::rules::flake8_use_pathlib::violations::{
 };
 use crate::settings::types::PythonVersion;
 
-pub fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr, parent: Option<&Expr>) {
+pub fn replaceable_by_pathlib(
+    checker: &mut Checker,
+    expr: &Expr,
+    func: &Expr,
+    parent: Option<&Expr>,
+) {
     if let Some(diagnostic_kind) =
         checker
-            .resolve_call_path(expr)
+            .resolve_call_path(func)
             .and_then(|call_path| match call_path.as_slice() {
                 ["os", "path", "abspath"] => Some(PathlibAbspath.into()),
                 ["os", "chmod"] => Some(PathlibChmod.into()),
@@ -58,7 +63,7 @@ pub fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr, parent: Option
         let mut diagnostic =
             Diagnostic::new::<DiagnosticKind>(diagnostic_kind, Range::from_located(expr));
         if checker.patch(diagnostic.kind.rule()) {
-            if let Some(fix) = pathlib_fix(checker, &diagnostic.kind, expr, parent) {
+            if let Some(fix) = pathlib_fix(checker, &diagnostic.kind, expr, func, parent) {
                 diagnostic.amend(fix);
             }
         }

diff --git a/...e8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH124_py_path_1.py.snap b/...e8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH124_py_path_1.py.snap
@@ -1,5 +1,5 @@
 ---
-source: src/rules/flake8_use_pathlib/mod.rs
+source: crates/ruff/src/rules/flake8_use_pathlib/mod.rs
 expression: diagnostics
 ---
 - kind:
@@ -9,7 +9,7 @@ expression: diagnostics
     column: 4
   end_location:
     row: 3
-    column: 17
+    column: 27
   fix: ~
   parent: ~
 
diff --git a/...e8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH124_py_path_2.py.snap b/...e8_use_pathlib/snapshots/ruff__rules__flake8_use_pathlib__tests__PTH124_py_path_2.py.snap
@@ -1,5 +1,5 @@
 ---
-source: src/rules/flake8_use_pathlib/mod.rs
+source: crates/ruff/src/rules/flake8_use_pathlib/mod.rs
 expression: diagnostics
 ---
 - kind:
@@ -9,7 +9,7 @@ expression: diagnostics
     column: 4
   end_location:
     row: 3
-    column: 8
+    column: 16
   fix: ~
   parent: ~