Change --fix-only exit semantics to mirror --fix (#4146)

astral-sh · May 4, 2023 · b71cc37 · b71cc37
1 parent 7171281
commit b71cc37
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 6 deletions.
diff --git a/BREAKING_CHANGES.md b/BREAKING_CHANGES.md
@@ -1,5 +1,16 @@
 # Breaking Changes
 
+## 0.0.265
+
+### `--fix-only` now exits with a zero exit code, unless `--exit-non-zero-on-fix` is specified ([#4146](https://github.com/charliermarsh/ruff/pull/4146))
+
+Previously, `--fix-only` would exit with a non-zero exit code if any fixes were applied. This
+behavior was inconsistent with `--fix`, and further, meant that `--exit-non-zero-on-fix` was
+effectively ignored when `--fix-only` was specified.
+
+Now, `--fix-only` will exit with a zero exit code, unless `--exit-non-zero-on-fix` is specified,
+in which case it will exit with a non-zero exit code if any fixes were applied.
+
 ## 0.0.260
 
 ### Fixes are now represented as a list of edits ([#3709](https://github.com/charliermarsh/ruff/pull/3709))

diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs
@@ -278,17 +278,33 @@ fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
         }
 
         if !cli.exit_zero {
-            if cli.diff || fix_only {
+            if cli.diff {
+                // If we're printing a diff, we always want to exit non-zero if there are
+                // any fixable violations (since we've printed the diff, but not applied the
+                // fixes).
                 if !diagnostics.fixed.is_empty() {
                     return Ok(ExitStatus::Failure);
                 }
-            } else if cli.exit_non_zero_on_fix {
-                if !diagnostics.fixed.is_empty() || !diagnostics.messages.is_empty() {
-                    return Ok(ExitStatus::Failure);
+            } else if fix_only {
+                // If we're only fixing, we want to exit zero (since we've fixed all fixable
+                // violations), unless we're explicitly asked to exit non-zero on fix.
+                if cli.exit_non_zero_on_fix {
+                    if !diagnostics.fixed.is_empty() {
+                        return Ok(ExitStatus::Failure);
+                    }
                 }
             } else {
-                if !diagnostics.messages.is_empty() {
-                    return Ok(ExitStatus::Failure);
+                // If we're running the linter (not just fixing), we want to exit non-zero if
+                // there are any violations, unless we're explicitly asked to exit zero on
+                // fix.
+                if cli.exit_non_zero_on_fix {
+                    if !diagnostics.fixed.is_empty() || !diagnostics.messages.is_empty() {
+                        return Ok(ExitStatus::Failure);
+                    }
+                } else {
+                    if !diagnostics.messages.is_empty() {
+                        return Ok(ExitStatus::Failure);
+                    }
                 }
             }
         }