linter: improve prefer-robust-stmts error messages (#461)

also update internal vscode extension readme

and fix typo in playground linter source name `tea` instead of `squawk`

Author: sbdchd

.vscode/extensions/squawk-dev/README.md

@@ -2,6 +2,30 @@
 
 Extension to make developing squawk a little nicer.
 
+## Install
+
+1. install deps and build
+
+```sh
+cd .vscode/extensions/squawk-dev
+pnpm install
+pnpm run compile
+```
+
+2. enable extension in vscode
+
+In the command palette run:
+
+```
+Extensions: Show Recommended Extensions
+```
+
+You should then see the `recipeyak-dev` extension in the results.
+
+Press the `Install Workspace Extension` button.
+
+Done!
+
 ## Features
 
 ### Jump from snapshot to source file

crates/squawk_linter/src/rules/prefer_robust_stmts.rs

@@ -30,6 +30,12 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse<SourceFile>) {
         return;
     }
 
+    enum ActionErrorMessage {
+        IfExists,
+        IfNotExists,
+        None,
+    }
+
     for item in file.items() {
         match item {
             ast::Item::Begin(_) => {
@@ -40,7 +46,7 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse<SourceFile>) {
             }
             ast::Item::AlterTable(alter_table) => {
                 for action in alter_table.actions() {
-                    match &action {
+                    let message_type = match &action {
                         ast::AlterTableAction::DropConstraint(drop_constraint) => {
                             if let Some(constraint_name) = drop_constraint.name_ref() {
                                 constraint_names.insert(
@@ -51,11 +57,13 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse<SourceFile>) {
                             if drop_constraint.if_exists().is_some() {
                                 continue;
                             }
+                            ActionErrorMessage::IfExists
                         }
                         ast::AlterTableAction::AddColumn(add_column) => {
                             if add_column.if_not_exists().is_some() {
                                 continue;
                             }
+                            ActionErrorMessage::IfNotExists
                         }
                         ast::AlterTableAction::ValidateConstraint(validate_constraint) => {
                             if let Some(constraint_name) = validate_constraint.name_ref() {
@@ -65,6 +73,7 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse<SourceFile>) {
                                     continue;
                                 }
                             }
+                            ActionErrorMessage::None
                         }
                         ast::AlterTableAction::AddConstraint(add_constraint) => {
                             let constraint = add_constraint.constraint();
@@ -78,22 +87,36 @@ pub(crate) fn prefer_robust_stmts(ctx: &mut Linter, parse: &Parse<SourceFile>) {
                                     }
                                 }
                             }
+                            ActionErrorMessage::None
                         }
                         ast::AlterTableAction::DropColumn(drop_column) => {
                             if drop_column.if_exists().is_some() {
                                 continue;
                             }
+                            ActionErrorMessage::IfExists
                         }
-                        _ => (),
-                    }
+                        _ => ActionErrorMessage::None,
+                    };
 
                     if inside_transaction {
                         continue;
                     }
 
+                    let message =  match message_type {
+                        ActionErrorMessage::IfExists => {
+                            "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.".to_string()
+                        },
+                        ActionErrorMessage::IfNotExists => {
+                            "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.".to_string()
+                        },
+                        ActionErrorMessage::None => {
+                            "Missing transaction, the migration can't be rerun if it fails part way through.".to_string()
+                        },
+                    };
+
                     ctx.report(Violation::new(
                         Rule::PreferRobustStmts,
-                    "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.".into(),
+                        message,
                         action.syntax().text_range(),
                         None,
                     ));
@@ -547,7 +570,20 @@ ALTER TABLE IF EXISTS test DISABLE ROW LEVEL SECURITY;
 ALTER TABLE "app_email" DROP CONSTRAINT IF EXISTS "email_uniq";
 ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
 -- this second add constraint should error because it's not robust
-ALTER TABLE "app_email" ADD CONSTRAINT "email_uniq" UNIQUE USING INDEX "email_idx";
+ALTER TABLE "app_email" ADD CONSTRAIN "email_uniq" UNIQUE USING INDEX "email_idx";
+        "#;
+        let file = squawk_syntax::SourceFile::parse(sql);
+        let mut linter = Linter::from([Rule::PreferRobustStmts]);
+        let errors = linter.lint(file, sql);
+        assert_ne!(errors.len(), 0);
+        assert_debug_snapshot!(errors);
+    }
+
+    #[test]
+    fn alter_column_set_not_null() {
+        let sql = r#"
+select 1; -- so we don't skip checking
+alter table t alter column c set not null;
         "#;
         let file = squawk_syntax::SourceFile::parse(sql);
         let mut linter = Linter::from([Rule::PreferRobustStmts]);

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__alter_column_set_not_null.snap

@@ -0,0 +1,12 @@
+---
+source: crates/squawk_linter/src/rules/prefer_robust_stmts.rs
+expression: errors
+---
+[
+    Violation {
+        code: PreferRobustStmts,
+        message: "Missing transaction, the migration can't be rerun if it fails part way through.",
+        text_range: 54..81,
+        help: None,
+    },
+]

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__alter_table_drop_column_err.snap

@@ -5,7 +5,7 @@ expression: errors
 [
     Violation {
         code: PreferRobustStmts,
-        message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
+        message: "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.",
         text_range: 54..75,
         help: None,
     },

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__alter_table_drop_constraint_err.snap

@@ -5,7 +5,7 @@ expression: errors
 [
     Violation {
         code: PreferRobustStmts,
-        message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
+        message: "Missing `IF EXISTS`, the migration can't be rerun if it fails part way through.",
         text_range: 63..93,
         help: None,
     },

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__disable_row_level_security_err.snap

@@ -5,7 +5,7 @@ expression: errors
 [
     Violation {
         code: PreferRobustStmts,
-        message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
+        message: "Missing transaction, the migration can't be rerun if it fails part way through.",
         text_range: 63..89,
         help: None,
     },

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__double_add_after_drop_err.snap

@@ -6,7 +6,7 @@ expression: errors
     Violation {
         code: PreferRobustStmts,
         message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
-        text_range: 240..298,
+        text_range: 240..297,
         help: None,
     },
 ]

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__enable_row_level_security_err.snap

@@ -5,7 +5,7 @@ expression: errors
 [
     Violation {
         code: PreferRobustStmts,
-        message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
+        message: "Missing transaction, the migration can't be rerun if it fails part way through.",
         text_range: 63..88,
         help: None,
     },

crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__prefer_robust_stmts__test__enable_row_level_security_without_exists_check_err.snap

@@ -5,7 +5,7 @@ expression: errors
 [
     Violation {
         code: PreferRobustStmts,
-        message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.",
+        message: "Missing transaction, the migration can't be rerun if it fails part way through.",
         text_range: 53..78,
         help: None,
     },

playground/src/App.tsx

@@ -246,7 +246,7 @@ function Editor({
     }
     const model = editorRef.current?.getModel()
     if (model != null) {
-      monaco.editor.setModelMarkers(model, "tea", markers)
+      monaco.editor.setModelMarkers(model, "squawk", markers)
     }
   }, [markers])
 
@@ -375,7 +375,7 @@ function useMarkers(text: string): Array<Marker> {
       // https://github.com/microsoft/monaco-editor/issues/1264
       // https://stackoverflow.com/questions/62362741/using-markdown-in-imarkerdata
       message: x.message,
-      source: "tea",
+      source: "squawk",
     }
   })
 }