use auto generated IDs (#218)

We can auto generate the IDs from our enum. We already use the auto generated IDs for parsing from the configuration file, so this makes everything consistent. I ran into an issue where `fmt::Display` was being recursive which I fixed with 85f626a.
sbdchd · Jun 16, 2022 · 709de74 · 709de74
1 parent 134d53d
commit 709de74
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## v0.13.2 - 2022-06-16
+
+## Changed
+
+- internal: use rule IDs based on serde annotations of `RuleViolationKind` (#218)
+
 ## v0.13.1 - 2022-06-15
 
 ## Fixed

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "squawk"
-version = "0.13.1"
+version = "0.13.2"
 authors = ["Steve Dignam <[email protected]>"]
 edition = "2018"
 license = "GPL-3.0"

diff --git a/linter/Cargo.toml b/linter/Cargo.toml
@@ -14,6 +14,7 @@ keywords = ["postgres", "sql", "linter"]
 [dependencies]
 serde = { version = "1.0", features = ["derive"] }
 squawk-parser = { version = "0.0.0", path = "../parser" }
+serde_plain = "1.0"
 lazy_static = "1.4.0"
 
 [dev-dependencies]

diff --git a/linter/src/lib.rs b/linter/src/lib.rs
@@ -21,7 +21,6 @@ use std::collections::HashSet;
 
 #[derive(Clone)]
 pub struct SquawkRule {
-    pub id: String,
     pub name: RuleViolationKind,
     func: fn(&[RawStmt]) -> Vec<RuleViolation>,
     pub messages: Vec<ViolationMessage>,
@@ -31,7 +30,6 @@ lazy_static! {
     pub static ref RULES: Vec<SquawkRule> = vec![
     // see ChangingColumnType
     SquawkRule {
-        id: "adding-field-with-default".into(),
         name: RuleViolationKind::AddingFieldWithDefault,
         func: adding_field_with_default,
         messages: vec![
@@ -45,7 +43,6 @@ lazy_static! {
         ],
     },
     SquawkRule {
-        id: "adding-foreign-key-constraint".into(),
         name: RuleViolationKind::AddingForeignKeyConstraint,
         func: adding_foreign_key_constraint,
         messages: vec![
@@ -57,7 +54,6 @@ lazy_static! {
     },
     // usually paired with a DEFAULT
     SquawkRule {
-        id: "adding-not-nullable-field".into(),
         name: RuleViolationKind::AddingNotNullableField,
         func: adding_not_nullable_field,
         messages: vec![
@@ -69,7 +65,6 @@ lazy_static! {
         ],
     },
     SquawkRule {
-        id: "adding-serial-primary-key-field".into(),
         name: RuleViolationKind::AddingSerialPrimaryKeyField,
         func: adding_primary_key_constraint,
         messages: vec![
@@ -83,7 +78,6 @@ lazy_static! {
         ],
     },
     SquawkRule {
-        id: "ban-char-field".into(),
         name: RuleViolationKind::BanCharField,
         func: ban_char_type,
         messages: vec![
@@ -93,7 +87,6 @@ lazy_static! {
         ]
     },
     SquawkRule {
-        id: "ban-drop-column".into(),
         name: RuleViolationKind::BanDropColumn,
         func: ban_drop_column,
         messages: vec![
@@ -103,7 +96,6 @@ lazy_static! {
         ],
     },
     SquawkRule {
-        id: "ban-drop-database".into(),
         name: RuleViolationKind::BanDropDatabase,
         func: ban_drop_database,
         messages: vec![
@@ -124,7 +116,6 @@ lazy_static! {
     // > double the disk space.
     // https://www.postgresql.org/docs/current/sql-altertable.html
     SquawkRule {
-        id: "changing-column-type".into(),
         name: RuleViolationKind::ChangingColumnType,
         func: changing_column_type,
         messages: vec![
@@ -155,7 +146,6 @@ lazy_static! {
     // > succeeds.
     // https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-NOTES
     SquawkRule {
-        id: "constraint-missing-not-valid".into(),
         name: RuleViolationKind::ConstraintMissingNotValid,
         func: constraint_missing_not_valid,
         messages: vec![
@@ -168,7 +158,6 @@ lazy_static! {
     // > lock.
     // https://www.postgresql.org/docs/current/sql-altertable.html
     SquawkRule {
-        id: "disallowed-unique-constraint".into(),
         name: RuleViolationKind::DisallowedUniqueConstraint,
         func: disallow_unique_constraint,
         messages: vec![
@@ -182,7 +171,6 @@ lazy_static! {
         ],
     },
     SquawkRule {
-        id: "prefer-robust-stmts".into(),
         name: RuleViolationKind::PreferRobustStmts,
         func: prefer_robust_stmts,
         messages: vec![
@@ -193,7 +181,6 @@ lazy_static! {
     },
     // see ConstraintMissingNotValid for more docs
     SquawkRule {
-        id: "prefer-text-field".into(),
         name: RuleViolationKind::PreferTextField,
         func: prefer_text_field,
         messages: vec![
@@ -211,7 +198,6 @@ lazy_static! {
     // > no effect on the stored data.
     // https://www.postgresql.org/docs/10/sql-altertable.html
     SquawkRule {
-        id: "renaming-column".into(),
         name: RuleViolationKind::RenamingColumn,
         func: renaming_column,
         messages: vec![
@@ -222,7 +208,6 @@ lazy_static! {
     },
     // see RenamingColumn rule
     SquawkRule {
-        id: "renaming-table".into(),
         name: RuleViolationKind::RenamingTable,
         func: renaming_table,
         messages: vec![
@@ -233,7 +218,6 @@ lazy_static! {
     },
     // https://www.postgresql.org/docs/10/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY
     SquawkRule {
-        id: "require-concurrent-index-creation".into(),
         name: RuleViolationKind::RequireConcurrentIndexCreation,
         func: require_concurrent_index_creation,
         messages: vec![
@@ -247,7 +231,6 @@ lazy_static! {
     },
     // https://www.postgresql.org/docs/10/sql-dropindex.html
     SquawkRule {
-        id: "require-concurrent-index-deletion".into(),
         name: RuleViolationKind::RequireConcurrentIndexDeletion,
         func: require_concurrent_index_deletion,
         messages: vec![
@@ -284,12 +267,13 @@ pub fn check_sql(
 #[cfg(test)]
 mod test_rules {
     use super::*;
+    use insta::{assert_debug_snapshot, assert_display_snapshot};
     use std::convert::TryFrom;
     use std::str::FromStr;
 
     #[test]
     fn rules_should_be_sorted() {
-        let original_rules: Vec<String> = RULES.clone().into_iter().map(|x| x.id).collect();
+        let original_rules: Vec<String> = RULES.iter().map(|x| x.name.to_string()).collect();
         let mut sorted_rule_ids = original_rules.clone();
         sorted_rule_ids.sort();
         assert_eq!(original_rules, sorted_rule_ids);
@@ -304,6 +288,17 @@ mod test_rules {
             assert_eq!(RuleViolationKind::try_from(rule_str.as_ref()), Ok(rule));
         }
     }
+    /// Ensure rule names don't change
+    #[test]
+    fn test_rule_names_debug_snap() {
+        let rule_names: Vec<String> = RULES.iter().map(|r| r.name.to_string()).collect();
+        assert_debug_snapshot!(rule_names);
+    }
+    #[test]
+    fn test_rule_names_display_snap() {
+        let rule_names: Vec<String> = RULES.iter().map(|r| r.name.to_string()).collect();
+        assert_display_snapshot!(rule_names.join("\n"));
+    }
 
     /// Ensure we stort the resulting violations by where they occur in the file.
     #[test]

diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap
@@ -0,0 +1,22 @@
+---
+source: linter/src/lib.rs
+expression: rule_names
+---
+[
+    "adding-field-with-default",
+    "adding-foreign-key-constraint",
+    "adding-not-nullable-field",
+    "adding-serial-primary-key-field",
+    "ban-char-field",
+    "ban-drop-column",
+    "ban-drop-database",
+    "changing-column-type",
+    "constraint-missing-not-valid",
+    "disallowed-unique-constraint",
+    "prefer-robust-stmts",
+    "prefer-text-field",
+    "renaming-column",
+    "renaming-table",
+    "require-concurrent-index-creation",
+    "require-concurrent-index-deletion",
+]
diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap
@@ -0,0 +1,20 @@
+---
+source: linter/src/lib.rs
+expression: "rule_names.join(\"\\n\")"
+---
+adding-field-with-default
+adding-foreign-key-constraint
+adding-not-nullable-field
+adding-serial-primary-key-field
+ban-char-field
+ban-drop-column
+ban-drop-database
+changing-column-type
+constraint-missing-not-valid
+disallowed-unique-constraint
+prefer-robust-stmts
+prefer-text-field
+renaming-column
+renaming-table
+require-concurrent-index-creation
+require-concurrent-index-deletion
diff --git a/linter/src/violations.rs b/linter/src/violations.rs
@@ -5,34 +5,48 @@ use serde::{Deserialize, Serialize};
 pub use squawk_parser::ast::Span;
 
 #[derive(Debug, PartialEq, Clone, Serialize, Hash, Eq, Deserialize)]
-#[serde(rename_all = "kebab-case")]
 pub enum RuleViolationKind {
+    #[serde(rename = "require-concurrent-index-creation")]
     RequireConcurrentIndexCreation,
+    #[serde(rename = "require-concurrent-index-deletion")]
     RequireConcurrentIndexDeletion,
+    #[serde(rename = "constraint-missing-not-valid")]
     ConstraintMissingNotValid,
+    #[serde(rename = "adding-field-with-default")]
     AddingFieldWithDefault,
+    #[serde(rename = "adding-foreign-key-constraint")]
     AddingForeignKeyConstraint,
+    #[serde(rename = "changing-column-type")]
     ChangingColumnType,
+    #[serde(rename = "adding-not-nullable-field")]
     AddingNotNullableField,
+    #[serde(rename = "adding-serial-primary-key-field")]
     AddingSerialPrimaryKeyField,
+    #[serde(rename = "renaming-column")]
     RenamingColumn,
+    #[serde(rename = "renaming-table")]
     RenamingTable,
+    #[serde(rename = "disallowed-unique-constraint")]
     DisallowedUniqueConstraint,
+    #[serde(rename = "ban-drop-database")]
     BanDropDatabase,
+    #[serde(rename = "prefer-text-field")]
     PreferTextField,
+    #[serde(rename = "prefer-robust-stmts")]
     PreferRobustStmts,
+    #[serde(rename = "ban-char-field")]
     BanCharField,
+    #[serde(rename = "ban-drop-column")]
     BanDropColumn,
 }
 
 impl std::fmt::Display for RuleViolationKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let rule = RULES
-            .iter()
-            .find(|rule| rule.name == *self)
-            .expect("We should always find ourself");
-
-        write!(f, "{}", rule.id)
+        write!(
+            f,
+            "{}",
+            serde_plain::to_string(self).map_err(|_| std::fmt::Error)?
+        )
     }
 }
 
@@ -50,16 +64,7 @@ impl std::fmt::Display for UnknownRuleName {
 impl std::str::FromStr for RuleViolationKind {
     type Err = UnknownRuleName;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        RULES
-            .iter()
-            .find_map(|rule| {
-                if rule.id == s {
-                    Some(rule.name.clone())
-                } else {
-                    None
-                }
-            })
-            .ok_or(UnknownRuleName { val: s.to_string() })
+        serde_plain::from_str(s).map_err(|_| UnknownRuleName { val: s.to_string() })
     }
 }
 

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "squawk-cli",
-  "version": "0.13.1",
+  "version": "0.13.2",
   "description": "linter for PostgreSQL, focused on migrations",
   "repository": "[email protected]:sbdchd/squawk.git",
   "author": "Steve Dignam <[email protected]>",