github: support github annotations

crates/squawk/src/github.rs

@@ -1,12 +1,13 @@
 use crate::UploadToGithubArgs;
 use crate::config::Config;
-use crate::reporter::{CheckReport, fmt_tty_violation};
+use crate::reporter::{CheckReport, fmt_github_annotations, fmt_tty_violation};
 use crate::{file_finding::find_paths, reporter::check_files};
 use anyhow::{Result, anyhow, bail};
 use console::strip_ansi_codes;
 use log::info;
 use squawk_github::{GitHubApi, actions, app, comment_on_pr};
 use squawk_linter::{Rule, Version};
+use std::io;
 
 const VERSION: &str = env!("CARGO_PKG_VERSION");
 
@@ -135,6 +136,10 @@ pub fn check_and_comment_on_pr(
         COMMENT_HEADER,
     )?;
 
+    let stdout = io::stdout();
+    let mut handle = stdout.lock();
+    fmt_github_annotations(&mut handle, &file_results)?;
+
     let violations: usize = file_results.iter().map(|f| f.violations.len()).sum();
 
     if fail_on_violations && violations > 0 {
@@ -257,6 +262,8 @@ SELECT 1;
                 message: "Adding a NOT NULL field requires exclusive locks and table rewrites."
                     .to_string(),
                 help: Some("Make the field nullable.".to_string()),
+                column_end: 0,
+                line_end: 1,
             }],
         }];
 

crates/squawk/src/main.rs

@@ -255,6 +255,8 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log
                     debug(&mut handle, &found_paths, read_stdin, &kind, opts.verbose)?;
                 } else {
                     let reporter = opts.reporter.unwrap_or(Reporter::Tty);
+                    let github_annotations = std::env::var("GITHUB_ACTIONS").is_ok()
+                        && !std::env::var("SQUAWK_DISABLE_GITHUB_ANNOTATIONS").is_ok();
                     let exit_code = check_and_dump_files(
                         &mut handle,
                         &found_paths,
@@ -264,6 +266,7 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log
                         pg_version,
                         assume_in_transaction,
                         &reporter,
+                        github_annotations,
                     )?;
                     return Ok(exit_code);
                 }

crates/squawk/src/reporter.rs

@@ -42,10 +42,14 @@ fn check_sql(
     for e in parse_errors {
         let range_start = e.range().start();
         let line_col = line_index.line_col(range_start);
+        let range_end = e.range().end();
+        let line_end = line_index.line_col(range_end);
         violations.push(ReportViolation {
             file: path.to_string(),
             line: line_col.line as usize,
+            line_end: line_end.line as usize,
             column: line_col.col as usize,
+            column_end: line_end.col as usize,
             level: ViolationLevel::Error,
             help: None,
             range: e.range(),
@@ -56,10 +60,14 @@ fn check_sql(
     for e in errors {
         let range_start = e.text_range.start();
         let line_col = line_index.line_col(range_start);
+        let range_end = e.text_range.end();
+        let line_end = line_index.line_col(range_end);
         violations.push(ReportViolation {
             file: path.to_string(),
             line: line_col.line as usize,
+            line_end: line_end.line as usize,
             column: line_col.col as usize,
+            column_end: line_end.col as usize,
             range: e.text_range,
             help: e.help,
             level: ViolationLevel::Warning,
@@ -158,6 +166,7 @@ pub fn check_and_dump_files<W: io::Write>(
     pg_version: Option<Version>,
     assume_in_transaction: bool,
     reporter: &Reporter,
+    github_annotations: bool,
 ) -> Result<ExitCode> {
     let violations = check_files(
         path_patterns,
@@ -170,7 +179,7 @@ pub fn check_and_dump_files<W: io::Write>(
 
     let ok = violations.iter().map(|x| x.violations.len()).sum::<usize>() == 0;
 
-    print_violations(f, violations, reporter)?;
+    print_violations(f, violations, reporter, github_annotations)?;
 
     Ok(if ok {
         ExitCode::SUCCESS
@@ -273,6 +282,8 @@ pub struct ReportViolation {
     pub message: String,
     pub help: Option<String>,
     pub rule_name: String,
+    pub column_end: usize,
+    pub line_end: usize,
 }
 
 fn fmt_gcc<W: io::Write>(f: &mut W, reports: &[CheckReport]) -> Result<()> {
@@ -324,6 +335,30 @@ fn fmt_json<W: io::Write>(f: &mut W, reports: Vec<CheckReport>) -> Result<()> {
     Ok(())
 }
 
+pub fn fmt_github_annotations<W: io::Write>(f: &mut W, reports: &[CheckReport]) -> Result<()> {
+    for report in reports {
+        for violation in &report.violations {
+            let level = match violation.level {
+                ViolationLevel::Warning => "warning",
+                ViolationLevel::Error => "error",
+            };
+
+            writeln!(
+                f,
+                "::{level} file={file},line={line},col={col},endLine={line_end},endColumn={col_end},title={title}::{message}",
+                file = violation.file,
+                line = violation.line + 1,
+                line_end = violation.line_end + 1,
+                col = violation.column,
+                col_end = violation.column_end,
+                title = violation.rule_name,
+                message = violation.message,
+            )?;
+        }
+    }
+    Ok(())
+}
+
 #[derive(Debug)]
 pub struct CheckReport {
     pub filename: String,
@@ -335,7 +370,11 @@ pub fn print_violations<W: io::Write>(
     writer: &mut W,
     reports: Vec<CheckReport>,
     reporter: &Reporter,
+    github_annotations: bool,
 ) -> Result<()> {
+    if github_annotations {
+        fmt_github_annotations(writer, &reports)?;
+    }
     match reporter {
         Reporter::Gcc => fmt_gcc(writer, &reports),
         Reporter::Json => fmt_json(writer, reports),
@@ -387,6 +426,7 @@ SELECT 1;
             &mut buff,
             vec![check_sql(sql, filename, &[], None, false)],
             &Reporter::Gcc,
+            false,
         );
         assert!(res.is_ok());
 
@@ -400,6 +440,31 @@ SELECT 1;
         "###);
     }
 
+    #[test]
+    fn display_violations_tty_and_github_annotations() {
+        let sql = r#" 
+   ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
+-- multi line example
+ALTER TABLE "core_foo" 
+  ADD COLUMN "bar" 
+    integer NOT NULL;
+SELECT 1;
+"#;
+        let filename = "main.sql";
+        let mut buff = Vec::new();
+
+        let res = print_violations(
+            &mut buff,
+            vec![check_sql(sql, filename, &[], None, false)],
+            &Reporter::Tty,
+            true,
+        );
+
+        assert!(res.is_ok());
+        // remove the color codes so tests behave in CI as they do locally
+        assert_snapshot!(strip_ansi_codes(&String::from_utf8_lossy(&buff)));
+    }
+
     #[test]
     fn display_violations_tty() {
         let sql = r#" 
@@ -414,6 +479,7 @@ SELECT 1;
             &mut buff,
             vec![check_sql(sql, filename, &[], None, false)],
             &Reporter::Tty,
+            false,
         );
 
         assert!(res.is_ok());
@@ -429,6 +495,7 @@ SELECT 1;
             &mut buff,
             vec![check_sql(sql, "main.sql", &[], None, false)],
             &Reporter::Tty,
+            false,
         );
 
         assert!(res.is_ok());
@@ -450,10 +517,11 @@ SELECT 1;
             &mut buff,
             vec![check_sql(sql, filename, &[], None, false)],
             &Reporter::Json,
+            false,
         );
 
         assert!(res.is_ok());
-        assert_snapshot!(String::from_utf8_lossy(&buff), @r###"[{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field"},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts"},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int"},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field"},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts"},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int"}]"###);
+        assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":53,"line_end":1},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":47,"line_end":2}]"#);
     }
 
     #[test]

crates/squawk/src/snapshots/squawk__reporter__test_check_files__check_files_invalid_syntax.snap

@@ -1,5 +1,5 @@
 ---
-source: crates/cli/src/reporter.rs
+source: crates/squawk/src/reporter.rs
 expression: val
 ---
-[{"column":6,"file":"test.sql","help":null,"level":"Error","line":1,"message":"expected SEMICOLON","rule_name":"syntax-error"},{"column":7,"file":"test.sql","help":null,"level":"Error","line":1,"message":"expected command, found ERROR","rule_name":"syntax-error"}]
+[{"column":6,"column_end":6,"file":"test.sql","help":null,"level":"Error","line":1,"line_end":1,"message":"expected SEMICOLON","rule_name":"syntax-error"},{"column":7,"column_end":7,"file":"test.sql","help":null,"level":"Error","line":1,"line_end":1,"message":"expected command, found ERROR","rule_name":"syntax-error"}]

crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap

@@ -0,0 +1,55 @@
+---
+source: crates/squawk/src/reporter.rs
+expression: "strip_ansi_codes(&String::from_utf8_lossy(&buff))"
+---
+::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.
+::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.
+::warning file=main.sql,line=2,col=46,endLine=2,endColumn=53,title=prefer-bigint-over-int::Using 32-bit integer fields can result in hitting the max `int` limit.
+::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.
+::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.
+::warning file=main.sql,line=6,col=4,endLine=6,endColumn=11,title=prefer-bigint-over-int::Using 32-bit integer fields can result in hitting the max `int` limit.
+warning[adding-required-field]: Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.
+ --> main.sql:2:30
+  |
+2 |    ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
+  |                              ---------------------------------
+  |
+  = help: Make the field nullable or add a non-VOLATILE DEFAULT
+warning[prefer-robust-stmts]: Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.
+ --> main.sql:2:30
+  |
+2 |    ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
+  |                              ---------------------------------
+  |
+warning[prefer-bigint-over-int]: Using 32-bit integer fields can result in hitting the max `int` limit.
+ --> main.sql:2:47
+  |
+2 |    ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
+  |                                               -------
+  |
+  = help: Use 64-bit integer values instead to prevent hitting this limit.
+warning[adding-required-field]: Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.
+ --> main.sql:5:3
+  |
+5 | /   ADD COLUMN "bar" 
+6 | |     integer NOT NULL;
+  | |____________________-
+  |
+  = help: Make the field nullable or add a non-VOLATILE DEFAULT
+warning[prefer-robust-stmts]: Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.
+ --> main.sql:5:3
+  |
+5 | /   ADD COLUMN "bar" 
+6 | |     integer NOT NULL;
+  | |____________________-
+  |
+warning[prefer-bigint-over-int]: Using 32-bit integer fields can result in hitting the max `int` limit.
+ --> main.sql:6:5
+  |
+6 |     integer NOT NULL;
+  |     -------
+  |
+  = help: Use 64-bit integer values instead to prevent hitting this limit.
+
+Find detailed examples and solutions for each rule at https://squawkhq.com/docs/rules
+Found 6 issues in 1 file (checked 1 source file)

crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap

@@ -1,5 +1,5 @@
 ---
-source: crates/cli/src/reporter.rs
+source: crates/squawk/src/reporter.rs
 expression: "check_sql(sql, filename, &[], None, false)"
 ---
 CheckReport {
@@ -17,6 +17,8 @@ CheckReport {
                 "Make the field nullable or add a non-VOLATILE DEFAULT",
             ),
             rule_name: "adding-required-field",
+            column_end: 62,
+            line_end: 2,
         },
         ReportViolation {
             file: "main.sql",
@@ -27,6 +29,8 @@ CheckReport {
             message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
             help: None,
             rule_name: "prefer-robust-stmts",
+            column_end: 62,
+            line_end: 2,
         },
         ReportViolation {
             file: "main.sql",
@@ -39,6 +43,8 @@ CheckReport {
                 "Use 64-bit integer values instead to prevent hitting this limit.",
             ),
             rule_name: "prefer-bigint-over-int",
+            column_end: 53,
+            line_end: 2,
         },
         ReportViolation {
             file: "main.sql",
@@ -51,6 +57,8 @@ CheckReport {
                 "Make the field nullable or add a non-VOLATILE DEFAULT",
             ),
             rule_name: "adding-required-field",
+            column_end: 56,
+            line_end: 3,
         },
         ReportViolation {
             file: "main.sql",
@@ -61,6 +69,8 @@ CheckReport {
             message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
             help: None,
             rule_name: "prefer-robust-stmts",
+            column_end: 56,
+            line_end: 3,
         },
         ReportViolation {
             file: "main.sql",
@@ -73,6 +83,8 @@ CheckReport {
                 "Use 64-bit integer values instead to prevent hitting this limit.",
             ),
             rule_name: "prefer-bigint-over-int",
+            column_end: 47,
+            line_end: 3,
         },
     ],
 }