validate configured rules (#216)

Now when a rule name is provided, it is validated against our rule set. If an unknown rule is specified, we raise an error.

I've also update some of the help docs to have more descriptive value names.

Fixes #214

CHANGELOG.md

@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## v0.13.0 - 2022-06-06
+
+## Added
+
+- added validation for configured rules. (#216)
+
+### Changed
+
+- Change help menu value names. (#216)
+
 ## v0.12.0 - 2022-05-27
 
 ## Added

README.md

@@ -65,7 +65,7 @@ squawk
 Find problems in your SQL
 
 USAGE:
-    squawk [FLAGS] [OPTIONS] [paths]... [SUBCOMMAND]
+    squawk [FLAGS] [OPTIONS] [path]... [SUBCOMMAND]
 
 FLAGS:
     -h, --help
@@ -77,24 +77,33 @@ FLAGS:
     -V, --version
             Prints version information
 
+        --verbose
+            Enable debug logging output
+
 
 OPTIONS:
-        --dump-ast <dump-ast>
-            Output AST in JSON [possible values: Raw, Parsed]
+    -c, --config <config-path>
+            Path to the squawk config file (.squawk.toml)
+
+        --dump-ast <ast-format>
+            Output AST in JSON [possible values: Raw, Parsed, Debug]
 
-    -e, --exclude <exclude>...
+    -e, --exclude <rule>...
             Exclude specific warnings
 
             For example: --exclude=require-concurrent-index-creation,ban-drop-database
-        --explain <explain>
+        --explain <rule>
             Provide documentation on the given rule
 
         --reporter <reporter>
             Style of error reporting [possible values: Tty, Gcc, Json]
 
+        --stdin-filepath <filepath>
+            Path to use in reporting for stdin
+
 
 ARGS:
-    <paths>...
+    <path>...
             Paths to search
 
 

cli/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "squawk"
-version = "0.12.0"
+version = "0.13.0"
 authors = ["Steve Dignam <[email protected]>"]
 edition = "2018"
 license = "GPL-3.0"

cli/src/config.rs

@@ -1,5 +1,6 @@
 use log::info;
 use serde::Deserialize;
+use squawk_linter::violations::RuleViolationKind;
 use std::{env, io, path::Path, path::PathBuf};
 
 const FILE_NAME: &str = ".squawk.toml";
@@ -38,7 +39,7 @@ impl std::fmt::Display for ConfigError {
 #[derive(Debug, Default, Deserialize)]
 pub struct Config {
     #[serde(default)]
-    pub excluded_rules: Vec<String>,
+    pub excluded_rules: Vec<RuleViolationKind>,
 }
 
 impl Config {

cli/src/main.rs

@@ -16,6 +16,7 @@ use atty::Stream;
 use config::Config;
 use log::info;
 use simplelog::CombinedLogger;
+use squawk_linter::violations::RuleViolationKind;
 use std::io;
 use std::path::PathBuf;
 use std::process;
@@ -35,26 +36,32 @@ fn exit<E: std::fmt::Display, T>(res: Result<T, E>, msg: &str) -> ! {
 #[derive(StructOpt, Debug)]
 struct Opt {
     /// Paths to search
+    #[structopt(value_name = "path")]
     paths: Vec<String>,
     /// Exclude specific warnings
     ///
     /// For example:
     /// --exclude=require-concurrent-index-creation,ban-drop-database
-    #[structopt(short, long, use_delimiter = true)]
-    exclude: Option<Vec<String>>,
+    #[structopt(
+        short = "e",
+        long = "exclude",
+        value_name = "rule",
+        use_delimiter = true
+    )]
+    excluded_rules: Option<Vec<RuleViolationKind>>,
     /// List all available rules
     #[structopt(long)]
     list_rules: bool,
     /// Provide documentation on the given rule
-    #[structopt(long)]
+    #[structopt(long, value_name = "rule")]
     explain: Option<String>,
     /// Output AST in JSON
-    #[structopt(long, possible_values = &DumpAstOption::variants(), case_insensitive = true)]
+    #[structopt(long,value_name ="ast-format", possible_values = &DumpAstOption::variants(), case_insensitive = true)]
     dump_ast: Option<DumpAstOption>,
     /// Style of error reporting
     #[structopt(long, possible_values = &Reporter::variants(), case_insensitive = true)]
     reporter: Option<Reporter>,
-    #[structopt(long)]
+    #[structopt(long, value_name = "filepath")]
     /// Path to use in reporting for stdin
     stdin_filepath: Option<String>,
     #[structopt(subcommand)]
@@ -82,7 +89,7 @@ fn main() {
 
     let conf = Config::parse(opts.config_path);
     // the --exclude flag completely overrides the configuration file.
-    let excluded_rules = if let Some(excluded_rules) = opts.exclude {
+    let excluded_rules = if let Some(excluded_rules) = opts.excluded_rules {
         excluded_rules
     } else {
         conf.unwrap_or_else(|e| {

cli/src/reporter.rs

@@ -132,7 +132,7 @@ pub fn check_files(
     paths: &[String],
     is_stdin: bool,
     stdin_path: Option<String>,
-    excluded_rules: &[String],
+    excluded_rules: &[RuleViolationKind],
 ) -> Result<Vec<ViolationContent>, CheckFilesError> {
     let mut output_violations = vec![];
 
@@ -587,8 +587,8 @@ mod test_reporter {
 ALTER TABLE "core_foo" ADD COLUMN "bar" integer NOT NULL;
 SELECT 1;
 "#;
-        let violations =
-            check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid sql should parse");
+        let violations = check_sql(sql, &[RuleViolationKind::PreferRobustStmts])
+            .expect("valid sql should parse");
 
         let filename = "main.sql";
 
@@ -614,8 +614,8 @@ SELECT 1;
 ALTER TABLE "core_foo" ADD COLUMN "bar" integer NOT NULL;
 SELECT 1;
 "#;
-        let violations =
-            check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid sql should parse");
+        let violations = check_sql(sql, &[RuleViolationKind::PreferRobustStmts])
+            .expect("valid sql should parse");
         let filename = "main.sql";
         let mut buff = Vec::new();
 
@@ -651,8 +651,8 @@ SELECT 1;
 ALTER TABLE "core_foo" ADD COLUMN "bar" integer NOT NULL;
 SELECT 1;
 "#;
-        let violations =
-            check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid sql should parse");
+        let violations = check_sql(sql, &[RuleViolationKind::PreferRobustStmts])
+            .expect("valid sql should parse");
         let filename = "main.sql";
         let mut buff = Vec::new();
 
@@ -675,8 +675,8 @@ SELECT 1;
 ALTER TABLE "core_foo" ADD COLUMN "bar" integer NOT NULL;
 SELECT 1;
 "#;
-        let violations =
-            check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid sql should parse");
+        let violations = check_sql(sql, &[RuleViolationKind::PreferRobustStmts])
+            .expect("valid sql should parse");
 
         let filename = "main.sql";
         assert_debug_snapshot!(pretty_violations(violations, sql, filename));
@@ -687,8 +687,8 @@ SELECT 1;
     #[test]
     fn test_trimming_sql_newlines() {
         let sql = r#"ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;"#;
-        let violations =
-            check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid sql should parse");
+        let violations = check_sql(sql, &[RuleViolationKind::PreferRobustStmts])
+            .expect("valid sql should parse");
 
         assert_debug_snapshot!(violations, @r###"
         [

cli/src/subcommand.rs

@@ -1,6 +1,7 @@
 use crate::reporter::{check_files, get_comment_body, CheckFilesError};
 use log::info;
 use squawk_github::{actions, app, comment_on_pr, GithubError};
+use squawk_linter::violations::RuleViolationKind;
 use structopt::StructOpt;
 
 const VERSION: &str = env!("CARGO_PKG_VERSION");
@@ -57,7 +58,7 @@ pub enum Command {
         /// For example:
         /// --exclude=require-concurrent-index-creation,ban-drop-database
         #[structopt(short, long, use_delimiter = true)]
-        exclude: Option<Vec<String>>,
+        exclude: Option<Vec<RuleViolationKind>>,
         #[structopt(long, env = "SQUAWK_GITHUB_PRIVATE_KEY")]
         github_private_key: Option<String>,
         #[structopt(long, env = "SQUAWK_GITHUB_PRIVATE_KEY_BASE64")]
@@ -98,7 +99,7 @@ fn get_github_private_key(
     }
 }
 
-fn concat(a: &[String], b: &[String]) -> Vec<String> {
+fn concat(a: &[RuleViolationKind], b: &[RuleViolationKind]) -> Vec<RuleViolationKind> {
     // from: https://stackoverflow.com/a/53476705/3720597
     [a, b].concat()
 }
@@ -107,7 +108,7 @@ pub fn check_and_comment_on_pr(
     cmd: Command,
     is_stdin: bool,
     stdin_path: Option<String>,
-    root_cmd_exclude: &[String],
+    root_cmd_exclude: &[RuleViolationKind],
 ) -> Result<(), SquawkError> {
     let Command::UploadToGithub {
         paths,

docs/docs/cli.md

@@ -52,7 +52,7 @@ squawk
 Find problems in your SQL
 
 USAGE:
-    squawk [FLAGS] [OPTIONS] [paths]... [SUBCOMMAND]
+    squawk [FLAGS] [OPTIONS] [path]... [SUBCOMMAND]
 
 FLAGS:
     -h, --help          
@@ -69,32 +69,32 @@ FLAGS:
 
 
 OPTIONS:
-    -c, --config <config-path>               
+    -c, --config <config-path>         
             Path to the squawk config file (.squawk.toml)
 
-        --dump-ast <dump-ast>                
+        --dump-ast <ast-format>        
             Output AST in JSON [possible values: Raw, Parsed, Debug]
 
-    -e, --exclude <exclude>...               
+    -e, --exclude <rule>...            
             Exclude specific warnings
             
             For example: --exclude=require-concurrent-index-creation,ban-drop-database
-        --explain <explain>                  
+        --explain <rule>               
             Provide documentation on the given rule
 
-        --reporter <reporter>                
+        --reporter <reporter>          
             Style of error reporting [possible values: Tty, Gcc, Json]
 
-        --stdin-filepath <stdin-filepath>    
+        --stdin-filepath <filepath>    
             Path to use in reporting for stdin
 
 
 ARGS:
-    <paths>...    
+    <path>...    
             Paths to search
 
 
 SUBCOMMANDS:
     help                Prints this message or the help of the given subcommand(s)
-    upload-to-github    Comment on a PR with Squawk's results
+    upload-to-github    Comment on a PR with Squawk's results 
 ```

linter/src/lib.rs

@@ -18,7 +18,6 @@ use crate::violations::{RuleViolation, RuleViolationKind, ViolationMessage};
 use squawk_parser::ast::RawStmt;
 use squawk_parser::parse::parse_sql_query;
 use std::collections::HashSet;
-use std::convert::TryFrom;
 
 #[derive(Clone)]
 pub struct SquawkRule {
@@ -266,14 +265,11 @@ lazy_static! {
 
 pub fn check_sql(
     sql: &str,
-    excluded_rules: &[String],
+    excluded_rules: &[RuleViolationKind],
 ) -> Result<Vec<RuleViolation>, CheckSqlError> {
     let tree = parse_sql_query(sql)?;
 
-    let excluded_rules: HashSet<RuleViolationKind> = excluded_rules
-        .iter()
-        .filter_map(|s| RuleViolationKind::try_from(s.as_ref()).ok())
-        .collect();
+    let excluded_rules: HashSet<RuleViolationKind> = excluded_rules.iter().cloned().collect();
 
     let mut errs = vec![];
     for rule in RULES.iter().filter(|r| !excluded_rules.contains(&r.name)) {
@@ -288,6 +284,8 @@ pub fn check_sql(
 #[cfg(test)]
 mod test_rules {
     use super::*;
+    use std::convert::TryFrom;
+    use std::str::FromStr;
 
     #[test]
     fn rules_should_be_sorted() {
@@ -301,10 +299,9 @@ mod test_rules {
     fn test_parsing_rule_kind() {
         let rule_names = RULES.iter().map(|r| r.name.clone());
         for rule in rule_names {
-            assert_eq!(
-                RuleViolationKind::try_from(rule.to_string().as_ref()),
-                Ok(rule)
-            );
+            let rule_str = rule.to_string();
+            assert_eq!(RuleViolationKind::from_str(&rule_str), Ok(rule.clone()));
+            assert_eq!(RuleViolationKind::try_from(rule_str.as_ref()), Ok(rule));
         }
     }
 
@@ -316,7 +313,8 @@ mod test_rules {
   CREATE INDEX "field_name_idx" ON "table_name" ("field_name");
   "#;
 
-        let res = check_sql(sql, &["prefer-robust-stmts".into()]).expect("valid parsing of SQL");
+        let res =
+            check_sql(sql, &[RuleViolationKind::PreferRobustStmts]).expect("valid parsing of SQL");
         let mut prev_span_start = -1;
         for violation in &res {
             assert!(violation.span.start > prev_span_start);

linter/src/rules/adding_field_with_default.rs

@@ -34,7 +34,7 @@ pub fn adding_field_with_default(tree: &[RawStmt]) -> Vec<RuleViolation> {
 
 #[cfg(test)]
 mod test_rules {
-    use crate::check_sql;
+    use crate::{check_sql, violations::RuleViolationKind};
     use insta::assert_debug_snapshot;
 
     ///
@@ -62,7 +62,7 @@ ALTER TABLE "core_recipe" ALTER COLUMN "foo" SET DEFAULT 10;
 -- remove nullability
         "#;
 
-        assert_debug_snapshot!(check_sql(bad_sql, &["prefer-robust-stmts".into()]));
-        assert_debug_snapshot!(check_sql(ok_sql, &["prefer-robust-stmts".into()]));
+        assert_debug_snapshot!(check_sql(bad_sql, &[RuleViolationKind::PreferRobustStmts]));
+        assert_debug_snapshot!(check_sql(ok_sql, &[RuleViolationKind::PreferRobustStmts]));
     }
 }