fix: returned error codes on fix

Author: benfdking

crates/cli/Cargo.toml

@@ -24,6 +24,11 @@ name = "bench"
 path = "src/bin/bench.rs"
 bench = false
 
+
+[[test]]
+name = "fix_return_code"
+harness = false
+
 [[test]]
 name = "config_not_found"
 harness = false

crates/cli/src/commands_fix.rs

@@ -45,16 +45,26 @@ pub(crate) fn run_fix(
             }
         }
 
+        let any_unfixable_errors = result.paths.iter().any(|path| {
+            path.files
+                .iter()
+                .any(|file| !file.get_violations(Some(false)).is_empty())
+        });
+
         for linted_dir in result.paths {
             for mut file in linted_dir.files {
                 let path = std::mem::take(&mut file.path);
                 let write_buff = file.fix_string();
                 std::fs::write(path, write_buff).unwrap();
             }
         }
-
         linter.formatter_mut().unwrap().completion_message();
-        0
+
+        if any_unfixable_errors {
+            1
+        } else {
+            0
+        }
     }
 }
 

crates/cli/tests/configure_rule.rs

@@ -7,7 +7,6 @@ fn main() {
     configure_rule();
 }
 
-///
 fn configure_rule() {
     let profile = if cfg!(debug_assertions) {
         "debug"
@@ -51,7 +50,7 @@ fn configure_rule() {
             .arg("--config")
             .arg(&config)
             .arg(&sql_path);
-        cmd.current_dir(&cargo_folder);
+        cmd.current_dir(cargo_folder);
 
         // Set the HOME environment variable to the fake home directory
         cmd.env("HOME", PathBuf::from(env!("CARGO_MANIFEST_DIR")));

crates/cli/tests/fix_return_code.rs

@@ -0,0 +1,111 @@
+use core::str;
+use std::path::{Path, PathBuf};
+
+use assert_cmd::Command;
+
+fn main() {
+    fix_return_code();
+}
+
+fn fix_return_code() {
+    let profile = if cfg!(debug_assertions) {
+        "debug"
+    } else {
+        "release"
+    };
+
+    // Tests needed
+    // STDIN
+    // - Fix, do nothing -> 0
+    // - Fix, fix everything -> 0
+    // - Fix, fix some not all -> 1
+    // TODO File
+    // - Fix, do nothing -> 0
+    // - Fix, fix everything -> 0
+    // - Fix, fix some not all -> 1
+
+    let cargo_folder = Path::new(env!("CARGO_MANIFEST_DIR"));
+    let mut sqruff_path = PathBuf::from(cargo_folder);
+    sqruff_path.push(format!("../../target/{}/sqruff", profile));
+
+    // STDIN - do nothing
+    let mut cmd = Command::new(sqruff_path.clone());
+    cmd.env("HOME", PathBuf::from(env!("CARGO_MANIFEST_DIR")));
+    cmd.arg("fix").arg("-f").arg("human").arg("-");
+    cmd.current_dir(cargo_folder);
+    cmd.write_stdin("SELECT foo FROM bar;\n");
+
+    // Run the command and capture the output
+    // let assert = cmd.assert();
+    // let output = assert.get_output();
+
+    // assert!(output.status.code().unwrap().to_string() == "0");
+
+    // let stdout_str = str::from_utf8(&output.stdout).unwrap();
+    // let stderr_str = str::from_utf8(&output.stderr).unwrap();
+    // assert_eq!(stdout_str, "SELECT foo FROM bar;\n\n");
+    // assert_eq!(stderr_str, "");
+
+    // STDIN - nothing to fix
+    let config_file = cargo_folder.join("tests/fix_return_code/fix_everything.cfg");
+    let mut cmd = Command::new(sqruff_path.clone());
+    cmd.env("HOME", PathBuf::from(env!("CARGO_MANIFEST_DIR")));
+    cmd.arg("fix")
+        .arg("-f")
+        .arg("human")
+        .arg("--config")
+        .arg(&config_file)
+        .arg("-");
+    cmd.write_stdin("SELECT foo AS bar FROM tabs");
+
+    let assert = cmd.assert();
+    let output = assert.get_output();
+
+    let stdout_str = str::from_utf8(&output.stdout).unwrap();
+    let stderr_str = str::from_utf8(&output.stderr).unwrap();
+    assert_eq!(stdout_str, "SELECT foo AS bar FROM tabs\n");
+    assert_eq!(stderr_str, "");
+    assert_eq!(output.status.code().unwrap(), 0);
+
+    // STDIN - fix everything
+    let config_file = cargo_folder.join("tests/fix_return_code/fix_everything.cfg");
+    let mut cmd = Command::new(sqruff_path.clone());
+    cmd.env("HOME", PathBuf::from(env!("CARGO_MANIFEST_DIR")));
+    cmd.arg("fix")
+        .arg("-f")
+        .arg("human")
+        .arg("--config")
+        .arg(&config_file)
+        .arg("-");
+    cmd.write_stdin("SELECT foo bar FROM tabs");
+
+    let assert = cmd.assert();
+    let output = assert.get_output();
+
+    let stdout_str = str::from_utf8(&output.stdout).unwrap();
+    let stderr_str = str::from_utf8(&output.stderr).unwrap();
+    assert_eq!(stdout_str, "SELECT foo AS bar FROM tabs\n");
+    assert_eq!(stderr_str, "== [<string>] FAIL\nL:   1 | P:  12 | AL02 | Implicit/explicit aliasing of columns.\n                       | [aliasing.column]\n");
+    assert_eq!(output.status.code().unwrap(), 0);
+
+    // STDIN - fix some not all
+    let config_file = cargo_folder.join("tests/fix_return_code/fix_some.cfg");
+    let mut cmd = Command::new(sqruff_path.clone());
+    cmd.env("HOME", PathBuf::from(env!("CARGO_MANIFEST_DIR")));
+    cmd.arg("fix")
+        .arg("-f")
+        .arg("human")
+        .arg("--config")
+        .arg(&config_file)
+        .arg("-");
+    cmd.write_stdin("SELECT foo bar, * FROM tabs");
+
+    let assert = cmd.assert();
+    let output = assert.get_output();
+
+    let stdout_str = str::from_utf8(&output.stdout).unwrap();
+    let stderr_str = str::from_utf8(&output.stderr).unwrap();
+    assert_eq!(stdout_str, "SELECT foo AS bar, * FROM tabs\n");
+    assert_eq!(stderr_str, "== [<string>] FAIL\nL:   1 | P:   1 | AM04 | Outermost query should produce known number of columns.\n                       | [ambiguous.column_count]\nL:   1 | P:  12 | AL02 | Implicit/explicit aliasing of columns.\n                       | [aliasing.column]\n");
+    assert_eq!(output.status.code().unwrap(), 1);
+}

crates/cli/tests/fix_return_code/fix_everything.cfg

@@ -0,0 +1,2 @@
+[sqruff]
+rules = AL02

crates/cli/tests/fix_return_code/fix_some.cfg

@@ -0,0 +1,2 @@
+[sqruff]
+rules = AL02, AM04

crates/lib-core/src/errors.rs

@@ -56,7 +56,7 @@ impl SQLBaseError {
 
 impl SqlError for SQLBaseError {
     fn fixable(&self) -> bool {
-        false
+        self.fixable
     }
 
     fn rule_code(&self) -> Option<&'static str> {

crates/lib/src/core/linter/linted_file.rs

@@ -3,7 +3,7 @@ use std::ops::Range;
 use crate::core::rules::noqa::IgnoreMask;
 use itertools::Itertools;
 use rustc_hash::FxHashSet;
-use sqruff_lib_core::errors::SQLBaseError;
+use sqruff_lib_core::errors::{SQLBaseError, SqlError};
 use sqruff_lib_core::parser::segments::fix::FixPatch;
 use sqruff_lib_core::templaters::base::{RawFileSlice, TemplatedFile};
 
@@ -17,11 +17,11 @@ pub struct LintedFile {
 }
 
 impl LintedFile {
-    pub fn get_violations(&self, fixable: Option<bool>) -> Vec<SQLBaseError> {
-        if let Some(fixable) = fixable {
+    pub fn get_violations(&self, is_fixable: Option<bool>) -> Vec<SQLBaseError> {
+        if let Some(is_fixable) = is_fixable {
             self.violations
                 .iter()
-                .filter(|v| v.fixable == fixable)
+                .filter(|v| v.fixable() == is_fixable)
                 .cloned()
                 .collect_vec()
         } else {

crates/lib/src/rules/aliasing/al02.rs

@@ -43,6 +43,10 @@ impl Rule for RuleAL02 {
         Ok(rule.erased())
     }
 
+    fn is_fix_compatible(&self) -> bool {
+        true
+    }
+
     fn name(&self) -> &'static str {
         "aliasing.column"
     }

docs/rules.md

@@ -108,7 +108,7 @@ Implicit/explicit aliasing of columns.
 
 **Groups:** `all`, `core`, `aliasing`
 
-**Fixable:** No
+**Fixable:** Yes
 
 **Anti-pattern**