Skip to content

Commit b60ba75

Browse files
fennrMichaReiser
fennr
and
MichaReiser
authored
[flake8_use_pathlib]: Replace os.symlink with Path.symlink_to (PTH211) (#18337)
Co-authored-by: Micha Reiser <[email protected]>
1 parent 66ba1d8 commit b60ba75

File tree

8 files changed

+132
-14
lines changed

8 files changed

+132
-14
lines changed

‎crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH211.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import os
2+
from pathlib import Path
3+
4+
5+
os.symlink("usr/bin/python", "tmp/python")
6+
os.symlink(b"usr/bin/python", b"tmp/python")
7+
Path("tmp/python").symlink_to("usr/bin/python") # Ok
8+
9+
os.symlink("usr/bin/python", "tmp/python", target_is_directory=True)
10+
os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True)
11+
Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok
12+
13+
fd = os.open(".", os.O_RDONLY)
14+
os.symlink("source.txt", "link.txt", dir_fd=fd) # Ok: dir_fd is not supported by pathlib
15+
os.close(fd)

‎crates/ruff_linter/src/checkers/ast/analyze/expression.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1041,6 +1041,7 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
10411041
Rule::OsPathGetctime,
10421042
Rule::Glob,
10431043
Rule::OsListdir,
1044+
Rule::OsSymlink,
10441045
]) {
10451046
flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call);
10461047
}

‎crates/ruff_linter/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -934,6 +934,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
934934
(Flake8UsePathlib, "207") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::Glob),
935935
(Flake8UsePathlib, "208") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsListdir),
936936
(Flake8UsePathlib, "210") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::InvalidPathlibWithSuffix),
937+
(Flake8UsePathlib, "211") => (RuleGroup::Preview, rules::flake8_use_pathlib::violations::OsSymlink),
937938

938939
// flake8-logging-format
939940
(Flake8LoggingFormat, "001") => (RuleGroup::Stable, rules::flake8_logging_format::violations::LoggingStringFormat),

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ mod tests {
6666
#[test_case(Rule::OsListdir, Path::new("PTH208.py"))]
6767
#[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210.py"))]
6868
#[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210_1.py"))]
69+
#[test_case(Rule::OsSymlink, Path::new("PTH211.py"))]
6970
fn rules_pypath(rule_code: Rule, path: &Path) -> Result<()> {
7071
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
7172
let diagnostics = test_path(

‎crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::rules::flake8_use_pathlib::violations::{
1313
BuiltinOpen, Joiner, OsChmod, OsGetcwd, OsListdir, OsMakedirs, OsMkdir, OsPathAbspath,
1414
OsPathBasename, OsPathDirname, OsPathExists, OsPathExpanduser, OsPathIsabs, OsPathIsdir,
1515
OsPathIsfile, OsPathIslink, OsPathJoin, OsPathSamefile, OsPathSplitext, OsReadlink, OsRemove,
16-
OsRename, OsReplace, OsRmdir, OsStat, OsUnlink, PyPath,
16+
OsRename, OsReplace, OsRmdir, OsStat, OsSymlink, OsUnlink, PyPath,
1717
};
1818
use ruff_python_ast::PythonVersion;
1919

@@ -38,7 +38,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
3838
.arguments
3939
.find_argument_value("path", 0)
4040
.is_some_and(|expr| is_file_descriptor(expr, checker.semantic()))
41-
|| is_argument_non_default(&call.arguments, "dir_fd", 2)
41+
|| is_keyword_only_argument_non_default(&call.arguments, "dir_fd")
4242
{
4343
return;
4444
}
@@ -54,7 +54,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
5454
// 0 1 2
5555
// os.mkdir(path, mode=0o777, *, dir_fd=None)
5656
// ```
57-
if is_argument_non_default(&call.arguments, "dir_fd", 2) {
57+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
5858
return;
5959
}
6060
Diagnostic::new(OsMkdir, range)
@@ -68,8 +68,8 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
6868
// 0 1 2 3
6969
// os.rename(src, dst, *, src_dir_fd=None, dst_dir_fd=None)
7070
// ```
71-
if is_argument_non_default(&call.arguments, "src_dir_fd", 2)
72-
|| is_argument_non_default(&call.arguments, "dst_dir_fd", 3)
71+
if is_keyword_only_argument_non_default(&call.arguments, "src_dir_fd")
72+
|| is_keyword_only_argument_non_default(&call.arguments, "dst_dir_fd")
7373
{
7474
return;
7575
}
@@ -84,8 +84,8 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
8484
// 0 1 2 3
8585
// os.replace(src, dst, *, src_dir_fd=None, dst_dir_fd=None)
8686
// ```
87-
if is_argument_non_default(&call.arguments, "src_dir_fd", 2)
88-
|| is_argument_non_default(&call.arguments, "dst_dir_fd", 3)
87+
if is_keyword_only_argument_non_default(&call.arguments, "src_dir_fd")
88+
|| is_keyword_only_argument_non_default(&call.arguments, "dst_dir_fd")
8989
{
9090
return;
9191
}
@@ -99,7 +99,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
9999
// 0 1
100100
// os.rmdir(path, *, dir_fd=None)
101101
// ```
102-
if is_argument_non_default(&call.arguments, "dir_fd", 1) {
102+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
103103
return;
104104
}
105105
Diagnostic::new(OsRmdir, range)
@@ -112,7 +112,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
112112
// 0 1
113113
// os.remove(path, *, dir_fd=None)
114114
// ```
115-
if is_argument_non_default(&call.arguments, "dir_fd", 1) {
115+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
116116
return;
117117
}
118118
Diagnostic::new(OsRemove, range)
@@ -125,7 +125,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
125125
// 0 1
126126
// os.unlink(path, *, dir_fd=None)
127127
// ```
128-
if is_argument_non_default(&call.arguments, "dir_fd", 1) {
128+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
129129
return;
130130
}
131131
Diagnostic::new(OsUnlink, range)
@@ -155,7 +155,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
155155
.arguments
156156
.find_argument_value("path", 0)
157157
.is_some_and(|expr| is_file_descriptor(expr, checker.semantic()))
158-
|| is_argument_non_default(&call.arguments, "dir_fd", 1)
158+
|| is_keyword_only_argument_non_default(&call.arguments, "dir_fd")
159159
{
160160
return;
161161
}
@@ -202,6 +202,20 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
202202
["os", "path", "getmtime"] => Diagnostic::new(OsPathGetmtime, range),
203203
// PTH205
204204
["os", "path", "getctime"] => Diagnostic::new(OsPathGetctime, range),
205+
// PTH211
206+
["os", "symlink"] => {
207+
// `dir_fd` is not supported by pathlib, so check if there are non-default values.
208+
// Signature as of Python 3.13 (https://docs.python.org/3/library/os.html#os.symlink)
209+
// ```text
210+
// 0 1 2 3
211+
// os.symlink(src, dst, target_is_directory=False, *, dir_fd=None)
212+
// ```
213+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
214+
return;
215+
}
216+
Diagnostic::new(OsSymlink, range)
217+
}
218+
205219
// PTH123
206220
["" | "builtins", "open"] => {
207221
// `closefd` and `opener` are not supported by pathlib, so check if they are
@@ -248,7 +262,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
248262
// 0 1 2 3 4
249263
// glob.glob(pathname, *, root_dir=None, dir_fd=None, recursive=False, include_hidden=False)
250264
// ```
251-
if is_argument_non_default(&call.arguments, "dir_fd", 2) {
265+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
252266
return;
253267
}
254268

@@ -267,7 +281,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
267281
// 0 1 2 3 4
268282
// glob.iglob(pathname, *, root_dir=None, dir_fd=None, recursive=False, include_hidden=False)
269283
// ```
270-
if is_argument_non_default(&call.arguments, "dir_fd", 2) {
284+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
271285
return;
272286
}
273287

@@ -287,7 +301,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
287301
// 0 1
288302
// os.readlink(path, *, dir_fd=None)
289303
// ```
290-
if is_argument_non_default(&call.arguments, "dir_fd", 1) {
304+
if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") {
291305
return;
292306
}
293307
Diagnostic::new(OsReadlink, range)
@@ -303,6 +317,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
303317
}
304318
Diagnostic::new(OsListdir, range)
305319
}
320+
306321
_ => return,
307322
};
308323

@@ -348,3 +363,9 @@ fn is_argument_non_default(arguments: &ast::Arguments, name: &str, position: usi
348363
.find_argument_value(name, position)
349364
.is_some_and(|expr| !expr.is_none_literal_expr())
350365
}
366+
367+
fn is_keyword_only_argument_non_default(arguments: &ast::Arguments, name: &str) -> bool {
368+
arguments
369+
.find_keyword(name)
370+
.is_some_and(|keyword| !keyword.value.is_none_literal_expr())
371+
}

‎crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH211_PTH211.py.snap

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs
3+
---
4+
PTH211.py:5:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to`
5+
|
6+
5 | os.symlink("usr/bin/python", "tmp/python")
7+
| ^^^^^^^^^^ PTH211
8+
6 | os.symlink(b"usr/bin/python", b"tmp/python")
9+
7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok
10+
|
11+
12+
PTH211.py:6:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to`
13+
|
14+
5 | os.symlink("usr/bin/python", "tmp/python")
15+
6 | os.symlink(b"usr/bin/python", b"tmp/python")
16+
| ^^^^^^^^^^ PTH211
17+
7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok
18+
|
19+
20+
PTH211.py:9:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to`
21+
|
22+
7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok
23+
8 |
24+
9 | os.symlink("usr/bin/python", "tmp/python", target_is_directory=True)
25+
| ^^^^^^^^^^ PTH211
26+
10 | os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True)
27+
11 | Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok
28+
|
29+
30+
PTH211.py:10:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to`
31+
|
32+
9 | os.symlink("usr/bin/python", "tmp/python", target_is_directory=True)
33+
10 | os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True)
34+
| ^^^^^^^^^^ PTH211
35+
11 | Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok
36+
|

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,3 +1215,45 @@ impl Violation for OsListdir {
12151215
"Use `pathlib.Path.iterdir()` instead.".to_string()
12161216
}
12171217
}
1218+
1219+
/// ## What it does
1220+
/// Checks for uses of `os.symlink`.
1221+
///
1222+
/// ## Why is this bad?
1223+
/// `pathlib` offers a high-level API for path manipulation, as compared to
1224+
/// the lower-level API offered by `os.symlink`.
1225+
///
1226+
/// ## Example
1227+
/// ```python
1228+
/// import os
1229+
///
1230+
/// os.symlink("usr/bin/python", "tmp/python", target_is_directory=False)
1231+
/// ```
1232+
///
1233+
/// Use instead:
1234+
/// ```python
1235+
/// from pathlib import Path
1236+
///
1237+
/// Path("tmp/python").symlink_to("usr/bin/python")
1238+
/// ```
1239+
///
1240+
/// ## Known issues
1241+
/// While using `pathlib` can improve the readability and type safety of your code,
1242+
/// it can be less performant than the lower-level alternatives that work directly with strings,
1243+
/// especially on older versions of Python.
1244+
///
1245+
/// ## References
1246+
/// - [Python documentation: `Path.symlink_to`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.symlink_to)
1247+
/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/)
1248+
/// - [Correspondence between `os` and `pathlib`](https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module)
1249+
/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/)
1250+
/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/)
1251+
#[derive(ViolationMetadata)]
1252+
pub(crate) struct OsSymlink;
1253+
1254+
impl Violation for OsSymlink {
1255+
#[derive_message_formats]
1256+
fn message(&self) -> String {
1257+
"`os.symlink` should be replaced by `Path.symlink_to`".to_string()
1258+
}
1259+
}

‎ruff.schema.json

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)