Avoid adding duplicate text keyword to subprocess.run (#7112)

astral-sh · Sep 3, 2023 · c7217e3 · c7217e3
1 parent d702479
commit c7217e3
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 74 deletions.
diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP021.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP021.py
@@ -1,12 +1,10 @@
 import subprocess
-import subprocess as somename
 from subprocess import run
-from subprocess import run as anothername
 
+# Errors
 subprocess.run(["foo"], universal_newlines=True, check=True)
-somename.run(["foo"], universal_newlines=True)
-
+subprocess.run(["foo"], universal_newlines=True, text=True)
 run(["foo"], universal_newlines=True, check=False)
-anothername(["foo"], universal_newlines=True)
 
+# OK
 subprocess.run(["foo"], check=True)
diff --git a/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs b/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs
@@ -1,9 +1,9 @@
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_ast::{self as ast};
+use ruff_python_ast as ast;
 use ruff_text_size::Ranged;
-use ruff_text_size::{TextLen, TextRange};
 
+use crate::autofix::edits::{remove_argument, Parentheses};
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
 
@@ -58,13 +58,30 @@ pub(crate) fn replace_universal_newlines(checker: &mut Checker, call: &ast::Expr
         let Some(kwarg) = call.arguments.find_keyword("universal_newlines") else {
             return;
         };
-        let range = TextRange::at(kwarg.start(), "universal_newlines".text_len());
-        let mut diagnostic = Diagnostic::new(ReplaceUniversalNewlines, range);
+
+        let Some(arg) = kwarg.arg.as_ref() else {
+            return;
+        };
+
+        let mut diagnostic = Diagnostic::new(ReplaceUniversalNewlines, arg.range());
+
         if checker.patch(diagnostic.kind.rule()) {
-            diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
-                "text".to_string(),
-                range,
-            )));
+            if call.arguments.find_keyword("text").is_some() {
+                diagnostic.try_set_fix(|| {
+                    remove_argument(
+                        kwarg,
+                        &call.arguments,
+                        Parentheses::Preserve,
+                        checker.locator().contents(),
+                    )
+                    .map(Fix::suggested)
+                });
+            } else {
+                diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
+                    "text".to_string(),
+                    arg.range(),
+                )));
+            }
         }
         checker.diagnostics.push(diagnostic);
     }

diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP021.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP021.py.snap
@@ -1,83 +1,65 @@
 ---
 source: crates/ruff/src/rules/pyupgrade/mod.rs
 ---
-UP021.py:6:25: UP021 [*] `universal_newlines` is deprecated, use `text`
+UP021.py:5:25: UP021 [*] `universal_newlines` is deprecated, use `text`
   |
-4 | from subprocess import run as anothername
-5 | 
-6 | subprocess.run(["foo"], universal_newlines=True, check=True)
+4 | # Errors
+5 | subprocess.run(["foo"], universal_newlines=True, check=True)
   |                         ^^^^^^^^^^^^^^^^^^ UP021
-7 | somename.run(["foo"], universal_newlines=True)
+6 | subprocess.run(["foo"], universal_newlines=True, text=True)
+7 | run(["foo"], universal_newlines=True, check=False)
   |
   = help: Replace with `text` keyword argument
 
 ℹ Suggested fix
-3 3 | from subprocess import run
-4 4 | from subprocess import run as anothername
-5 5 | 
-6   |-subprocess.run(["foo"], universal_newlines=True, check=True)
-  6 |+subprocess.run(["foo"], text=True, check=True)
-7 7 | somename.run(["foo"], universal_newlines=True)
+2 2 | from subprocess import run
+3 3 | 
+4 4 | # Errors
+5   |-subprocess.run(["foo"], universal_newlines=True, check=True)
+  5 |+subprocess.run(["foo"], text=True, check=True)
+6 6 | subprocess.run(["foo"], universal_newlines=True, text=True)
+7 7 | run(["foo"], universal_newlines=True, check=False)
 8 8 | 
-9 9 | run(["foo"], universal_newlines=True, check=False)
 
-UP021.py:7:23: UP021 [*] `universal_newlines` is deprecated, use `text`
+UP021.py:6:25: UP021 [*] `universal_newlines` is deprecated, use `text`
   |
-6 | subprocess.run(["foo"], universal_newlines=True, check=True)
-7 | somename.run(["foo"], universal_newlines=True)
-  |                       ^^^^^^^^^^^^^^^^^^ UP021
-8 | 
-9 | run(["foo"], universal_newlines=True, check=False)
+4 | # Errors
+5 | subprocess.run(["foo"], universal_newlines=True, check=True)
+6 | subprocess.run(["foo"], universal_newlines=True, text=True)
+  |                         ^^^^^^^^^^^^^^^^^^ UP021
+7 | run(["foo"], universal_newlines=True, check=False)
   |
   = help: Replace with `text` keyword argument
 
 ℹ Suggested fix
-4 4 | from subprocess import run as anothername
-5 5 | 
-6 6 | subprocess.run(["foo"], universal_newlines=True, check=True)
-7   |-somename.run(["foo"], universal_newlines=True)
-  7 |+somename.run(["foo"], text=True)
+3 3 | 
+4 4 | # Errors
+5 5 | subprocess.run(["foo"], universal_newlines=True, check=True)
+6   |-subprocess.run(["foo"], universal_newlines=True, text=True)
+  6 |+subprocess.run(["foo"], text=True)
+7 7 | run(["foo"], universal_newlines=True, check=False)
 8 8 | 
-9 9 | run(["foo"], universal_newlines=True, check=False)
-10 10 | anothername(["foo"], universal_newlines=True)
-
-UP021.py:9:14: UP021 [*] `universal_newlines` is deprecated, use `text`
-   |
- 7 | somename.run(["foo"], universal_newlines=True)
- 8 | 
- 9 | run(["foo"], universal_newlines=True, check=False)
-   |              ^^^^^^^^^^^^^^^^^^ UP021
-10 | anothername(["foo"], universal_newlines=True)
-   |
-   = help: Replace with `text` keyword argument
-
-ℹ Suggested fix
-6  6  | subprocess.run(["foo"], universal_newlines=True, check=True)
-7  7  | somename.run(["foo"], universal_newlines=True)
-8  8  | 
-9     |-run(["foo"], universal_newlines=True, check=False)
-   9  |+run(["foo"], text=True, check=False)
-10 10 | anothername(["foo"], universal_newlines=True)
-11 11 | 
-12 12 | subprocess.run(["foo"], check=True)
+9 9 | # OK
 
-UP021.py:10:22: UP021 [*] `universal_newlines` is deprecated, use `text`
-   |
- 9 | run(["foo"], universal_newlines=True, check=False)
-10 | anothername(["foo"], universal_newlines=True)
-   |                      ^^^^^^^^^^^^^^^^^^ UP021
-11 | 
-12 | subprocess.run(["foo"], check=True)
-   |
-   = help: Replace with `text` keyword argument
+UP021.py:7:14: UP021 [*] `universal_newlines` is deprecated, use `text`
+  |
+5 | subprocess.run(["foo"], universal_newlines=True, check=True)
+6 | subprocess.run(["foo"], universal_newlines=True, text=True)
+7 | run(["foo"], universal_newlines=True, check=False)
+  |              ^^^^^^^^^^^^^^^^^^ UP021
+8 | 
+9 | # OK
+  |
+  = help: Replace with `text` keyword argument
 
 ℹ Suggested fix
-7  7  | somename.run(["foo"], universal_newlines=True)
-8  8  | 
-9  9  | run(["foo"], universal_newlines=True, check=False)
-10    |-anothername(["foo"], universal_newlines=True)
-   10 |+anothername(["foo"], text=True)
-11 11 | 
-12 12 | subprocess.run(["foo"], check=True)
+4 4 | # Errors
+5 5 | subprocess.run(["foo"], universal_newlines=True, check=True)
+6 6 | subprocess.run(["foo"], universal_newlines=True, text=True)
+7   |-run(["foo"], universal_newlines=True, check=False)
+  7 |+run(["foo"], text=True, check=False)
+8 8 | 
+9 9 | # OK
+10 10 | subprocess.run(["foo"], check=True)