refactor: unify LintingResult and Formatter APIs

Author: gvozdvmozgu

crates/cli-lib/src/commands_fix.rs

@@ -19,9 +19,8 @@ pub(crate) fn run_fix(
     let mut linter = linter(config, format, collect_parse_errors);
     let result = linter.lint_paths(paths, true, &ignorer);
 
-    if result.files.iter().all(|file| file.violations.is_empty()) {
-        let count_files = result.files.len();
-        println!("{} files processed, nothing to fix.", count_files);
+    if !result.has_violations() {
+        println!("{} files processed, nothing to fix.", result.len());
         0
     } else {
         if !force {
@@ -38,20 +37,19 @@ pub(crate) fn run_fix(
             }
         }
 
-        let any_unfixable_errors = result
-            .files
-            .iter()
-            .any(|file| !file.get_violations(Some(false)).is_empty());
+        let any_unfixable_errors = result.has_unfixable_violations();
+        let files = result.len();
 
-        for mut file in result.files {
+        for mut file in result {
             let path = std::mem::take(&mut file.path);
-            let write_buff = file.fix_string();
-            std::fs::write(path, write_buff).unwrap();
+            let fixed = file.fix_string();
+
+            std::fs::write(path, fixed).unwrap();
         }
 
-        linter.formatter_mut().unwrap().completion_message();
+        linter.formatter_mut().unwrap().completion_message(files);
 
-        if any_unfixable_errors { 1 } else { 0 }
+        any_unfixable_errors as i32
     }
 }
 
@@ -65,10 +63,10 @@ pub(crate) fn run_fix_stdin(
     let linter = linter(config, format, collect_parse_errors);
     let result = linter.lint_string(&read_in, None, true);
 
-    // print fixed to std out
-    let violations = result.get_violations(Some(false));
+    let has_unfixable_errors = result.has_unfixable_violations();
+
     println!("{}", result.fix_string());
 
     // if all fixable violations are fixable, return 0 else return 1
-    if violations.is_empty() { 0 } else { 1 }
+    has_unfixable_errors as i32
 }

crates/cli-lib/src/commands_lint.rs

@@ -13,7 +13,7 @@ pub(crate) fn run_lint(
     let mut linter = linter(config, format, collect_parse_errors);
     let result = linter.lint_paths(paths, false, &ignorer);
 
-    linter.formatter().unwrap().completion_message();
+    linter.formatter().unwrap().completion_message(result.len());
 
     result.has_violations() as i32
 }
@@ -28,7 +28,7 @@ pub(crate) fn run_lint_stdin(
     let linter = linter(config, format, collect_parse_errors);
     let result = linter.lint_string(&read_in, None, false);
 
-    linter.formatter().unwrap().completion_message();
+    linter.formatter().unwrap().completion_message(1);
 
     result.has_violations() as i32
 }

crates/cli-python/tests/json/test_fail_whitespace_before_comma.stdout

@@ -1 +1 @@
-{"tests/lint/test_fail_whitespace_before_comma.sql":[{"range":{"start":{"line":1,"character":8},"end":{"line":1,"character":8}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":9},"end":{"line":1,"character":9}},"message":"Unexpected whitespace before comma.","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Expected single whitespace between \",\" and \"4\".","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":12},"end":{"line":1,"character":12}},"message":"Files must end with a single trailing newline.","severity":"Warning","source":"sqruff","code":"LT12"}]}
+{"tests/lint/test_fail_whitespace_before_comma.sql":[{"range":{"start":{"line":1,"character":8},"end":{"line":1,"character":8}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":9},"end":{"line":1,"character":9}},"message":"Unexpected whitespace before comma.","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Expected single whitespace between \",\" and \"4\".","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":12},"end":{"line":1,"character":12}},"message":"Files must end with a single trailing newline.","severity":"Warning","source":"sqruff","code":"LT12"}]}

crates/cli/tests/json/test_fail_whitespace_before_comma.stdout

@@ -1 +1 @@
-{"tests/lint/test_fail_whitespace_before_comma.sql":[{"range":{"start":{"line":1,"character":8},"end":{"line":1,"character":8}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":9},"end":{"line":1,"character":9}},"message":"Unexpected whitespace before comma.","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Expected single whitespace between \",\" and \"4\".","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":12},"end":{"line":1,"character":12}},"message":"Files must end with a single trailing newline.","severity":"Warning","source":"sqruff","code":"LT12"}]}
+{"tests/lint/test_fail_whitespace_before_comma.sql":[{"range":{"start":{"line":1,"character":8},"end":{"line":1,"character":8}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":9},"end":{"line":1,"character":9}},"message":"Unexpected whitespace before comma.","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Column expression without alias. Use explicit `AS` clause.","severity":"Warning","source":"sqruff","code":"AL03"},{"range":{"start":{"line":1,"character":11},"end":{"line":1,"character":11}},"message":"Expected single whitespace between \",\" and \"4\".","severity":"Warning","source":"sqruff","code":"LT01"},{"range":{"start":{"line":1,"character":12},"end":{"line":1,"character":12}},"message":"Files must end with a single trailing newline.","severity":"Warning","source":"sqruff","code":"LT12"}]}

crates/lib-wasm/src/lib.rs

@@ -94,17 +94,17 @@ impl Linter {
             None
         };
 
-        let mut result = self.base.lint_parsed(&tables, parsed, tool == Tool::Format);
-        let violations = &mut result.violations;
+        let result = self.base.lint_parsed(&tables, parsed, tool == Tool::Format);
+        let violations = result.violations();
 
         let diagnostics = violations
-            .iter_mut()
+            .iter()
             .map(|violation| {
                 let start = line_index.line_col(violation.source_slice.start.try_into().unwrap());
                 let end = line_index.line_col(violation.source_slice.end.try_into().unwrap());
 
                 Diagnostic {
-                    message: std::mem::take(&mut violation.description),
+                    message: violation.description.clone(),
                     start_line_number: start.line + 1,
                     start_column: start.col + 1,
                     end_line_number: end.line + 1,

crates/lib/src/cli/formatters.rs

@@ -1,30 +1,18 @@
 use super::utils::*;
 use std::borrow::Cow;
 use std::io::{Stderr, Write};
-use std::sync::atomic::AtomicUsize;
 
 use anstyle::{AnsiColor, Effects, Style};
 use itertools::enumerate;
 use sqruff_lib_core::errors::SQLBaseError;
 
-use crate::core::config::FluffConfig;
 use crate::core::linter::linted_file::LintedFile;
 
 const LIGHT_GREY: Style = AnsiColor::Black.on_default().effects(Effects::BOLD);
 
 pub trait Formatter: Send + Sync {
-    fn dispatch_template_header(
-        &self,
-        f_name: String,
-        linter_config: FluffConfig,
-        file_config: FluffConfig,
-    );
-
-    fn dispatch_parse_header(&self, f_name: String);
-
-    fn dispatch_file_violations(&self, linted_file: &LintedFile, only_fixable: bool);
-
-    fn completion_message(&self);
+    fn dispatch_file_violations(&self, linted_file: &LintedFile);
+    fn completion_message(&self, count: usize);
 }
 
 pub struct OutputStreamFormatter {
@@ -33,48 +21,27 @@ pub struct OutputStreamFormatter {
     filter_empty: bool,
     verbosity: i32,
     output_line_length: usize,
-    files_dispatched: AtomicUsize,
 }
 
 impl Formatter for OutputStreamFormatter {
-    fn dispatch_file_violations(&self, linted_file: &LintedFile, only_fixable: bool) {
+    fn dispatch_file_violations(&self, linted_file: &LintedFile) {
         if self.verbosity < 0 {
             return;
         }
 
-        let s = self.format_file_violations(
-            &linted_file.path,
-            linted_file.get_violations(only_fixable.then_some(true)),
-        );
+        let s = self.format_file_violations(linted_file.path(), linted_file.violations());
 
         self.dispatch(&s);
-        self.files_dispatched
-            .fetch_add(1, std::sync::atomic::Ordering::SeqCst);
     }
 
-    fn completion_message(&self) {
-        let count = self
-            .files_dispatched
-            .load(std::sync::atomic::Ordering::SeqCst);
-        let message = format!("The linter processed {count} file(s).\n");
-        self.dispatch(&message);
-
-        let message = if self.plain_output {
+    fn completion_message(&self, count: usize) {
+        self.dispatch(&format!("The linter processed {count} file(s).\n"));
+        self.dispatch(if self.plain_output {
             "All Finished\n"
         } else {
             "All Finished 📜 🎉\n"
-        };
-        self.dispatch(message);
-    }
-    fn dispatch_template_header(
-        &self,
-        _f_name: String,
-        _linter_config: FluffConfig,
-        _file_config: FluffConfig,
-    ) {
+        });
     }
-
-    fn dispatch_parse_header(&self, _f_name: String) {}
 }
 
 impl OutputStreamFormatter {
@@ -85,7 +52,6 @@ impl OutputStreamFormatter {
             filter_empty: true,
             verbosity,
             output_line_length: 80,
-            files_dispatched: 0.into(),
         }
     }
 
@@ -101,7 +67,7 @@ impl OutputStreamFormatter {
         }
     }
 
-    fn format_file_violations(&self, fname: &str, mut violations: Vec<SQLBaseError>) -> String {
+    fn format_file_violations(&self, fname: &str, violations: &[SQLBaseError]) -> String {
         let mut text_buffer = String::new();
 
         let show = !violations.is_empty();
@@ -113,12 +79,6 @@ impl OutputStreamFormatter {
         }
 
         if show {
-            violations.sort_by(|a, b| {
-                a.line_no
-                    .cmp(&b.line_no)
-                    .then_with(|| a.line_pos.cmp(&b.line_pos))
-            });
-
             for violation in violations {
                 let text = self.format_violation(violation, self.output_line_length);
                 text_buffer.push_str(&text);
@@ -148,12 +108,7 @@ impl OutputStreamFormatter {
         format!("== [{filename}] {status}")
     }
 
-    fn format_violation(
-        &self,
-        violation: impl Into<SQLBaseError>,
-        max_line_length: usize,
-    ) -> String {
-        let violation: SQLBaseError = violation.into();
+    fn format_violation(&self, violation: &SQLBaseError, max_line_length: usize) -> String {
         let mut desc = violation.desc().to_string();
 
         let line_elem = format!("{:4}", violation.line_no);
@@ -217,10 +172,7 @@ impl Status {
 mod tests {
     use anstyle::AnsiColor;
     use fancy_regex::Regex;
-    use sqruff_lib_core::dialects::syntax::SyntaxKind;
-    use sqruff_lib_core::errors::{ErrorStructRule, SQLLintError};
-    use sqruff_lib_core::parser::markers::PositionMarker;
-    use sqruff_lib_core::parser::segments::base::SegmentBuilder;
+    use sqruff_lib_core::errors::{ErrorStructRule, SQLBaseError};
 
     use super::OutputStreamFormatter;
     use crate::cli::formatters::split_string_on_spaces;
@@ -267,24 +219,19 @@ mod tests {
     fn test_cli_formatters_violation() {
         let formatter = mk_formatter();
 
-        let s = SegmentBuilder::token(0, "foobarbar", SyntaxKind::Word)
-            .with_position(PositionMarker::new(
-                10..19,
-                10..19,
-                "      \n\n  foobarbar".into(),
-                None,
-                None,
-            ))
-            .finish();
-
-        let mut v = SQLLintError::new("DESC", s, false, vec![]);
-
-        v.rule = Some(ErrorStructRule {
-            name: "some-name",
-            code: "DESC",
-        });
+        let v = SQLBaseError {
+            fixable: false,
+            line_no: 3,
+            line_pos: 3,
+            description: "DESC".into(),
+            rule: Some(ErrorStructRule {
+                name: "some-name",
+                code: "DESC",
+            }),
+            source_slice: 0..0,
+        };
 
-        let f = formatter.format_violation(v, 90);
+        let f = formatter.format_violation(&v, 90);
 
         assert_eq!(escape_ansi(&f), "L:   3 | P:   3 | DESC | DESC [some-name]");
     }

crates/lib/src/cli/github_annotation_native_formatter.rs

@@ -1,4 +1,3 @@
-use crate::core::config::FluffConfig;
 use crate::core::linter::linted_file::LintedFile;
 use std::io::{Stderr, Write};
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -29,39 +28,14 @@ impl GithubAnnotationNativeFormatter {
 }
 
 impl Formatter for GithubAnnotationNativeFormatter {
-    fn dispatch_template_header(
-        &self,
-        _f_name: String,
-        _linter_config: FluffConfig,
-        _file_config: FluffConfig,
-    ) {
-        // No-op
-    }
-
-    fn dispatch_parse_header(&self, _f_name: String) {
-        // No-op
-    }
-
-    fn dispatch_file_violations(&self, linted_file: &LintedFile, _only_fixable: bool) {
-        let mut violations = linted_file.get_violations(None);
-
-        violations.sort_by(|a, b| {
-            a.line_no
-                .cmp(&b.line_no)
-                .then_with(|| a.line_pos.cmp(&b.line_pos))
-                .then_with(|| {
-                    let b = b.rule.as_ref().unwrap().code;
-                    a.rule.as_ref().unwrap().code.cmp(b)
-                })
-        });
-
-        for violation in violations {
+    fn dispatch_file_violations(&self, linted_file: &LintedFile) {
+        for violation in linted_file.violations() {
             let message = format!(
                 "::error title=sqruff,file={},line={},col={}::{}: {}\n",
-                linted_file.path,
+                linted_file.path(),
                 violation.line_no,
                 violation.line_pos,
-                violation.rule.as_ref().unwrap().code,
+                violation.rule_code(),
                 violation.description
             );
 
@@ -70,7 +44,7 @@ impl Formatter for GithubAnnotationNativeFormatter {
         }
     }
 
-    fn completion_message(&self) {
+    fn completion_message(&self, _count: usize) {
         // No-op
     }
 }

crates/lib/src/cli/json.rs

@@ -1,6 +1,6 @@
 use std::sync::Mutex;
 
-use crate::core::{config::FluffConfig, linter::linted_file::LintedFile};
+use crate::core::linter::linted_file::LintedFile;
 
 use super::{
     formatters::Formatter,
@@ -13,30 +13,20 @@ pub struct JsonFormatter {
 }
 
 impl Formatter for JsonFormatter {
-    fn dispatch_file_violations(&self, linted_file: &LintedFile, only_fixable: bool) {
-        let violations = linted_file.get_violations(only_fixable.then_some(true));
+    fn dispatch_file_violations(&self, linted_file: &LintedFile) {
+        let violations = linted_file.violations();
         let mut lock = self.violations.lock().unwrap();
-        lock.entry(linted_file.path.clone()).or_default().extend(
+        lock.entry(linted_file.path().into()).or_default().extend(
             violations
                 .iter()
                 .map(|err| Diagnostic::from(err.clone()))
                 .collect::<Vec<_>>(),
         );
     }
 
-    fn completion_message(&self) {
+    fn completion_message(&self, _count: usize) {
         let lock = self.violations.lock().unwrap();
         let json = serde_json::to_string(&*lock).unwrap();
         println!("{}", json);
     }
-
-    fn dispatch_template_header(
-        &self,
-        _f_name: String,
-        _linter_config: FluffConfig,
-        _file_config: FluffConfig,
-    ) {
-    }
-
-    fn dispatch_parse_header(&self, _f_name: String) {}
 }