Skip to content

Commit

Permalink
Show custom message for Path.joinpath with starred arguments (#7852)
Browse files Browse the repository at this point in the history
Closes #7833.
  • Loading branch information
charliermarsh authored and konstin committed Oct 11, 2023
1 parent 1666c97 commit 7ee7593
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
fp.read()
open(p).close()
os.getcwdb(p)
os.path.join(p, *q)
os.sep.join(p, *q)

# https://github.com/astral-sh/ruff/issues/7620
def opener(path, flags):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::rules::flake8_use_pathlib::rules::{
Glob, OsPathGetatime, OsPathGetctime, OsPathGetmtime, OsPathGetsize,
};
use crate::rules::flake8_use_pathlib::violations::{
BuiltinOpen, OsChmod, OsGetcwd, OsMakedirs, OsMkdir, OsPathAbspath, OsPathBasename,
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,
Expand Down Expand Up @@ -60,12 +60,22 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, call: &ExprCall) {
["os", "path", "join"] => Some(
OsPathJoin {
module: "path".to_string(),
joiner: if call.arguments.args.iter().any(Expr::is_starred_expr) {
Joiner::Joinpath
} else {
Joiner::Slash
},
}
.into(),
),
["os", "sep", "join"] => Some(
OsPathJoin {
module: "sep".to_string(),
joiner: if call.arguments.args.iter().any(Expr::is_starred_expr) {
Joiner::Joinpath
} else {
Joiner::Slash
},
}
.into(),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ full_name.py:34:1: PTH123 `open()` should be replaced by `Path.open()`
34 | open(p).close()
| ^^^^ PTH123
35 | os.getcwdb(p)
36 | os.path.join(p, *q)
|

full_name.py:35:1: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
Expand All @@ -275,27 +276,46 @@ full_name.py:35:1: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
34 | open(p).close()
35 | os.getcwdb(p)
| ^^^^^^^^^^ PTH109
36 |
37 | # https://github.com/astral-sh/ruff/issues/7620
36 | os.path.join(p, *q)
37 | os.sep.join(p, *q)
|

full_name.py:44:1: PTH123 `open()` should be replaced by `Path.open()`
full_name.py:36:1: PTH118 `os.path.join()` should be replaced by `Path.joinpath()`
|
42 | open(p, closefd=False)
43 | open(p, opener=opener)
44 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
34 | open(p).close()
35 | os.getcwdb(p)
36 | os.path.join(p, *q)
| ^^^^^^^^^^^^ PTH118
37 | os.sep.join(p, *q)
|

full_name.py:37:1: PTH118 `os.sep.join()` should be replaced by `Path.joinpath()`
|
35 | os.getcwdb(p)
36 | os.path.join(p, *q)
37 | os.sep.join(p, *q)
| ^^^^^^^^^^^ PTH118
38 |
39 | # https://github.com/astral-sh/ruff/issues/7620
|

full_name.py:46:1: PTH123 `open()` should be replaced by `Path.open()`
|
44 | open(p, closefd=False)
45 | open(p, opener=opener)
46 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
| ^^^^ PTH123
45 | open(p, 'r', - 1, None, None, None, True, None)
46 | open(p, 'r', - 1, None, None, None, False, opener)
47 | open(p, 'r', - 1, None, None, None, True, None)
48 | open(p, 'r', - 1, None, None, None, False, opener)
|

full_name.py:45:1: PTH123 `open()` should be replaced by `Path.open()`
full_name.py:47:1: PTH123 `open()` should be replaced by `Path.open()`
|
43 | open(p, opener=opener)
44 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
45 | open(p, 'r', - 1, None, None, None, True, None)
45 | open(p, opener=opener)
46 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
47 | open(p, 'r', - 1, None, None, None, True, None)
| ^^^^ PTH123
46 | open(p, 'r', - 1, None, None, None, False, opener)
48 | open(p, 'r', - 1, None, None, None, False, opener)
|


24 changes: 19 additions & 5 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -799,8 +799,8 @@ impl Violation for OsPathIsabs {
/// ## 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 `Path` object
/// methods such as `Path.joinpath()` can improve readability over the `os`
/// module's counterparts (e.g., `os.path.join()`).
/// methods such as `Path.joinpath()` or the `/` operator can improve
/// readability over the `os` module's counterparts (e.g., `os.path.join()`).
///
/// Note that `os` functions may be preferable if performance is a concern,
/// e.g., in hot loops.
Expand Down Expand Up @@ -828,17 +828,31 @@ impl Violation for OsPathIsabs {
/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/)
#[violation]
pub struct OsPathJoin {
pub module: String,
pub(crate) module: String,
pub(crate) joiner: Joiner,
}

impl Violation for OsPathJoin {
#[derive_message_formats]
fn message(&self) -> String {
let OsPathJoin { module } = self;
format!("`os.{module}.join()` should be replaced by `Path` with `/` operator")
let OsPathJoin { module, joiner } = self;
match joiner {
Joiner::Slash => {
format!("`os.{module}.join()` should be replaced by `Path` with `/` operator")
}
Joiner::Joinpath => {
format!("`os.{module}.join()` should be replaced by `Path.joinpath()`")
}
}
}
}

#[derive(Debug, PartialEq, Eq)]
pub(crate) enum Joiner {
Slash,
Joinpath,
}

/// ## What it does
/// Checks for uses of `os.path.basename`.
///
Expand Down

0 comments on commit 7ee7593

Please sign in to comment.