Skip to content

Commit

Permalink
refactor: move dialect skipper logic into separate method
Browse files Browse the repository at this point in the history
- moves the logic for on which dialects to skip to separate method so
  that it can also be included in the docs
  • Loading branch information
benfdking committed Jul 21, 2024
1 parent 27748e7 commit 4f6904c
Show file tree
Hide file tree
Showing 12 changed files with 104 additions and 42 deletions.
4 changes: 4 additions & 0 deletions crates/cli/src/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ struct Rule {
pub fixable: bool,
pub long_description: &'static str,
pub groups: Vec<&'static str>,
pub has_dialects: bool,
pub dialects: Vec<&'static str>,
}

impl From<ErasedRule> for Rule {
Expand All @@ -53,6 +55,8 @@ impl From<ErasedRule> for Rule {
description: value.description(),
long_description: value.long_description(),
groups: value.groups().iter().map(|g| g.as_ref()).collect(),
has_dialects: value.dialect_skip().len() > 0,
dialects: value.dialect_skip().into_iter().map(|dialect| dialect.as_ref()).collect(),
}
}
}
3 changes: 2 additions & 1 deletion crates/cli/src/docs/generate_rule_docs_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ The following rules are available in this create. This list is generated from th

**Fixable:** {% if rule.fixable %}Yes{% else %}No{% endif %}
{{ rule.long_description }}
{% endfor %}
{% if rule.has_dialects %}**Dialects where this rule is skipped:** {% for dialect in rule.dialects %}`{{ dialect }}`{% if not loop.last %}, {%endif %}{% endfor %}
{% endif %}{% endfor %}
16 changes: 15 additions & 1 deletion crates/lib/src/core/dialects/init.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
use std::str::FromStr;

use strum_macros::AsRefStr;

use super::base::Dialect;

#[derive(strum_macros::EnumString, Debug, Clone, Copy, Default, Ord, PartialOrd, Eq, PartialEq)]
#[derive(
strum_macros::EnumString,
AsRefStr,
Debug,
Clone,
Copy,
Default,
Ord,
PartialOrd,
Eq,
PartialEq,
Hash
)]
#[strum(serialize_all = "snake_case")]
pub enum DialectKind {
#[default]
Expand Down
52 changes: 31 additions & 21 deletions crates/lib/src/core/rules/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use super::context::RuleContext;
use super::crawlers::{BaseCrawler, Crawler};
use crate::core::config::{FluffConfig, Value};
use crate::core::dialects::base::Dialect;
use crate::core::dialects::init::DialectKind;
use crate::core::errors::SQLLintError;
use crate::core::parser::segments::base::ErasedSegment;
use crate::helpers::{Config, IndexMap};
Expand Down Expand Up @@ -282,6 +283,12 @@ pub trait Rule: CloneRule + dyn_clone::DynClone + Debug + 'static + Send + Sync
/// element.
fn groups(&self) -> &'static [RuleGroups];

/// Returns the set of dialects for which a particular rule should be
/// skipped.
fn dialect_skip(&self) -> &'static [DialectKind] {
&[]
}

fn code(&self) -> &'static str {
let name = std::any::type_name::<Self>();
name.split("::").last().unwrap().strip_prefix("Rule").unwrap_or(name)
Expand Down Expand Up @@ -318,31 +325,34 @@ pub trait Rule: CloneRule + dyn_clone::DynClone + Debug + 'static + Send + Sync
let mut fixes = Vec::new();

for context in self.crawl_behaviour().crawl(root_context) {
let resp =
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| self.eval(context)));

let resp = match resp {
Ok(t) => t,
Err(_) => {
vs.push(SQLLintError::new("Unexpected exception. Could you open an issue at https://github.com/quarylabs/sqruff", tree.clone()));
return (vs, fixes);
}
};
// TODO Will to return a note that rules were skipped
if !self.dialect_skip().contains(&dialect.name) {
let resp =
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| self.eval(context)));

let resp = match resp {
Ok(t) => t,
Err(_) => {
vs.push(SQLLintError::new("Unexpected exception. Could you open an issue at https://github.com/quarylabs/sqruff", tree.clone()));
return (vs, fixes);
}
};

let mut new_lerrs = Vec::new();
let mut new_fixes = Vec::new();
let mut new_lerrs = Vec::new();
let mut new_fixes = Vec::new();

if resp.is_empty() {
// Assume this means no problems (also means no memory)
} else {
for elem in resp {
self.process_lint_result(elem, &mut new_lerrs, &mut new_fixes);
if resp.is_empty() {
// Assume this means no problems (also means no memory)
} else {
for elem in resp {
self.process_lint_result(elem, &mut new_lerrs, &mut new_fixes);
}
}
}

// Consume the new results
vs.extend(new_lerrs);
fixes.extend(new_fixes);
// Consume the new results
vs.extend(new_lerrs);
fixes.extend(new_fixes);
}
}

(vs, fixes)
Expand Down
20 changes: 20 additions & 0 deletions crates/lib/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,24 @@ mod tests {
}
})
}

#[test]
fn rule_skip_dialect_should_have_no_duplicates() {
rules().iter().for_each(|rule| {
let skips = rule.dialect_skip();
assert_eq!(skips.len(), skips.iter().unique().count());
})
}

#[test]
fn rule_skip_dialect_should_be_alphabetical() {
rules().iter().for_each(|rule| {
let skips = rule.dialect_skip();
for i in 1..skips.len() {
if skips[i].as_ref() < skips[i].as_ref() {
panic!("not in alphabetical order in rule {}", rule.code())
}
}
})
}
}
17 changes: 13 additions & 4 deletions crates/lib/src/rules/ambiguous/AM02.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,21 @@ SELECT a, b FROM table_2
fn groups(&self) -> &'static [RuleGroups] {
&[RuleGroups::All, RuleGroups::Core, RuleGroups::Ambiguous]
}
fn eval(&self, rule_cx: RuleContext) -> Vec<LintResult> {

fn dialect_skip(&self) -> &'static [DialectKind] {
// TODO: add ansi, hive, mysql, redshift
if !matches!(rule_cx.dialect.name, DialectKind::Ansi) {
return Vec::new();
}
// TODO This feels wrong and should bneed fixing
&[
DialectKind::Bigquery,
DialectKind::Postgres,
DialectKind::Snowflake,
DialectKind::Clickhouse,
DialectKind::Sparksql,
DialectKind::Duckdb,
]
}

fn eval(&self, rule_cx: RuleContext) -> Vec<LintResult> {
let raw = rule_cx.segment.raw();
let raw_upper = raw.to_uppercase();

Expand Down
2 changes: 1 addition & 1 deletion crates/lib/src/rules/convention/CV06.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ impl RuleCV06 {
None,
None,
))
}
};
}
None
}
Expand Down
11 changes: 6 additions & 5 deletions crates/lib/src/rules/references/RF01.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,13 @@ FROM foo
&[RuleGroups::All, RuleGroups::Core, RuleGroups::References]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
// ["bigquery", "databricks", "hive", "redshift", "soql", "sparksql"]
if matches!(&context.dialect.name, DialectKind::Bigquery | DialectKind::Sparksql) {
return Vec::new();
}
fn dialect_skip(&self) -> &'static [DialectKind] {
// TODO Add others when finished, whole list["bigquery", "databricks", "hive",
// "redshift", "soql", "sparksql"]
&[DialectKind::Bigquery, DialectKind::Sparksql]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
let query = Query::from_segment(&context.segment, context.dialect, None);
let mut violations = Vec::new();
let tmp;
Expand Down
8 changes: 4 additions & 4 deletions crates/lib/src/rules/references/RF03.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,12 +324,12 @@ FROM foo
&[RuleGroups::All, RuleGroups::References]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
fn dialect_skip(&self) -> &'static [DialectKind] {
// TODO: add hive, redshift"
if context.dialect.name == DialectKind::Bigquery {
return Vec::new();
}
&[DialectKind::Bigquery]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
let query: Query<()> = Query::from_segment(&context.segment, context.dialect, None);
let mut visited: AHashSet<ErasedSegment> = AHashSet::new();

Expand Down
2 changes: 1 addition & 1 deletion crates/lib/src/rules/structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pub mod ST04;
pub mod ST06;
pub mod ST07;
pub mod ST08;
mod ST09;
pub mod ST09;

pub fn rules() -> Vec<ErasedRule> {
use crate::core::rules::base::Erased as _;
Expand Down
8 changes: 4 additions & 4 deletions crates/lib/src/rules/structure/ST07.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ INNER JOIN table_b
&[RuleGroups::All, RuleGroups::Structure]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
if context.dialect.name == DialectKind::Clickhouse {
return Vec::new();
}
fn dialect_skip(&self) -> &'static [DialectKind] {
&[DialectKind::Clickhouse]
}

fn eval(&self, context: RuleContext) -> Vec<LintResult> {
let functional_context = FunctionalContext::new(context.clone());
let segment = functional_context.segment();
let parent_stack = functional_context.parent_stack();
Expand Down
3 changes: 3 additions & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -1789,6 +1789,7 @@ SELECT
FROM foo
```
**Dialects where this rule is skipped:** `bigquery`, `sparksql`
### references.qualification
Expand Down Expand Up @@ -1860,6 +1861,7 @@ SELECT
FROM foo
```
**Dialects where this rule is skipped:** `bigquery`
### references.keywords
Expand Down Expand Up @@ -2227,6 +2229,7 @@ FROM
INNER JOIN table_b
ON table_a.id = table_b.id
```
**Dialects where this rule is skipped:** `snowflake`

### structure.distinct

Expand Down

0 comments on commit 4f6904c

Please sign in to comment.