diff --git a/crates/sqlbuild-rules/src/dsl_headers/main/format_headers.rs b/crates/sqlbuild-rules/src/dsl_headers/main/format_headers.rs index 978bad4747..c4277c4252 100644 --- a/crates/sqlbuild-rules/src/dsl_headers/main/format_headers.rs +++ b/crates/sqlbuild-rules/src/dsl_headers/main/format_headers.rs @@ -16,7 +16,11 @@ pub fn format_headers( let headers: Vec = scan_headers(text, false); let trimmed: Vec = without_trailing_whitespace(text, &headers); let (relocated, faults) = relocate_leading_comment(&trimmed, config, &headers, parser)?; - let relocated_headers: Vec = scan_headers(&relocated, false); + let relocated_headers: Vec = if relocated.as_slice() == text { + headers + } else { + scan_headers(&relocated, false) + }; let wrapped: Vec = wrap_descriptions(&relocated, &relocated_headers, config.line_width, parser)?; Ok((wrap_audits(&wrapped, config.line_width, parser)?, faults)) diff --git a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs index c016eccf54..9e0072c389 100644 --- a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs +++ b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter.rs @@ -5,17 +5,24 @@ use polyglot_sql::tokens::{Token, TokenType}; use polyglot_sql::{ComplexityGuardOptions, Dialect, DialectType, ParseOptions}; use rayon::iter::{IntoParallelIterator, ParallelIterator}; +use crate::lint_text::constants::LINT_SENTINEL_PREFIX; use crate::sql_lint::_helpers::formatter_syntax::{cte_macro_indices, protect_syntax}; use crate::sql_lint::_helpers::printed_layout::{PrintedSql, WrapOptions, wrap_lines}; use crate::sql_lint::_helpers::token_layout::{ - AuthoredSql, KeywordRecase, OracleLayout, comments_in, neutralize_comments, oracle_layout, - print_authored_tokens, tokenize, verify_token_invariant, + AuthoredSql, KeywordRecase, OracleLayout, TokenTexts, comments_in, neutralize_comments, + oracle_layout, print_authored_tokens, tokenize, verify_printed_tokens, }; use crate::sql_lint::constants::{ CAST_TYPE_SEPARATOR_KEYWORD, CLOSE_PARENTHESIS, LINT_API_VERSION, OPEN_PARENTHESIS, }; use crate::sql_lint::models::{FormatRequest, FormatResponse}; +#[cfg(test)] +thread_local! { + /// Idempotency re-prints on this thread, for tests that assert which bodies are re-printed. + pub(crate) static RELAYOUTS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + const NOT_IDEMPOTENT_FAILURE: &str = "native formatter output is not idempotent"; const UNSUPPORTED_SQL_FAILURE: &str = "native formatter could not safely restore literal or cast spelling"; @@ -92,10 +99,11 @@ fn try_format_sql(request: FormatRequest) -> Result { wrap: wrap.as_ref(), }; let (formatted, formatted_tokens, oracle) = format_layout(&original, &layout, &parse_options)?; - if relayout((&formatted, &formatted_tokens), oracle.as_ref(), &layout)? != formatted { + let changed = formatted != original; + if changed && relayout((&formatted, &formatted_tokens), oracle.as_ref(), &layout)? != formatted + { return Err(NOT_IDEMPOTENT_FAILURE.to_string()); } - let changed = formatted != original; Ok(FormatResponse { version: LINT_API_VERSION, changed, @@ -137,10 +145,13 @@ fn format_layout( tokens: &tokens, comments: &comments, }; - let (printed, recases) = layout.print(&authored, &oracle)?; + let (printed, recases, authored_texts) = layout.print(&authored, &oracle)?; + if printed == sql { + return Ok((printed, tokens, Some(oracle))); + } let printed_tokens = tokenize(&printed, dialect)?; - verify_token_invariant( - (sql, &tokens), + verify_printed_tokens( + (sql, &tokens, &authored_texts), (&printed, &printed_tokens), dialect, &recases, @@ -154,6 +165,8 @@ fn relayout( oracle: Option<&OracleLayout>, layout: &Layout<'_>, ) -> Result { + #[cfg(test)] + RELAYOUTS.with(|relayouts| relayouts.set(relayouts.get() + 1)); let Some(oracle) = oracle.filter(|_| !tokens.is_empty()) else { return Ok(formatted.to_string()); }; @@ -177,19 +190,19 @@ impl Layout<'_> { &self, authored: &AuthoredSql<'_>, oracle: &OracleLayout, - ) -> Result<(String, BTreeMap), String> { - let (printed, recases) = print_authored_tokens(authored, oracle, self.dialect)?; + ) -> Result<(String, BTreeMap, TokenTexts), String> { + let (printed, recases, texts) = print_authored_tokens(authored, oracle, self.dialect)?; let no_widths: HashMap = HashMap::new(); let token_widths = self.wrap.map_or(&no_widths, |wrap| wrap.token_widths); let Some(mut sql) = PrintedSql::parse(&printed, self.dialect, token_widths)? else { - return Ok((printed, recases)); + return Ok((printed, recases, texts)); }; sql.arrange_clauses(self.wrap.map_or(usize::MAX, |wrap| wrap.line_width)); if let Some(wrap) = self.wrap { wrap_lines(&mut sql, wrap.line_width); } sql.lead_operators_of_multiline_operands(); - Ok((sql.render(), recases)) + Ok((sql.render(), recases, texts)) } } @@ -222,6 +235,9 @@ fn layout_oracle( } fn layout_cte_macros(mut formatted: String, dialect: &Dialect) -> Result { + if !formatted.contains(LINT_SENTINEL_PREFIX) { + return Ok(formatted); + } let tokens = dialect .tokenize(&formatted) .map_err(|error| error.to_string())?; diff --git a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter_syntax.rs b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter_syntax.rs index a6863ca386..e59eb02800 100644 --- a/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter_syntax.rs +++ b/crates/sqlbuild-rules/src/sql_lint/_helpers/formatter_syntax.rs @@ -1,3 +1,4 @@ +use crate::lint_text::constants::LINT_SENTINEL_PREFIX; use crate::sql_lint::_helpers::engine::token_depths; use crate::sql_lint::constants::{ CTE_MACRO_PLACEHOLDER_LITERAL, POSITION_ARGUMENT_SEPARATOR, POSTFIX_CAST_PREFIX_TOKEN_COUNT, @@ -8,6 +9,8 @@ use std::collections::{HashMap, HashSet}; pub(super) struct ProtectedSql { pub sql: String, + /// The lower-case prefix every sentinel name starts with. + prefix: String, fragments: Vec<(String, String)>, in_functions: HashSet, } @@ -19,6 +22,14 @@ impl ProtectedSql { .map_err(|error| error.to_string())?; let mut positions: HashMap> = HashMap::new(); for (index, token) in tokens.iter().enumerate() { + let prefixed = token + .text + .as_bytes() + .get(..self.prefix.len()) + .is_some_and(|start| start.eq_ignore_ascii_case(self.prefix.as_bytes())); + if !prefixed { + continue; + } positions .entry(token.text.to_ascii_lowercase()) .or_default() @@ -101,9 +112,14 @@ pub(super) fn protect_syntax( expressions: bool, ) -> Result { let mut prefix = "__sqlbuild_format_".to_string(); - while sql.to_ascii_lowercase().contains(&prefix) { + let lowercase: String = sql.to_ascii_lowercase(); + while lowercase.contains(&prefix) { prefix.push('_'); } + let mut cast_type_ends: HashMap = HashMap::new(); + for (start, end) in cast_types { + let _ = cast_type_ends.entry(*start).or_insert(*end); + } let mut ranges: Vec<(usize, usize, String)> = Vec::new(); let cte_macros = cte_macro_indices(tokens); let depths = token_depths(tokens); @@ -112,23 +128,23 @@ pub(super) fn protect_syntax( let mut index = 0; while index < tokens.len() { let token = &tokens[index]; - let sentinel = format!("{prefix}{}__", ranges.len()); + let sentinel = || format!("{prefix}{}__", ranges.len()); if separators.contains(&index) { ranges.push((token.span.start, token.span.end, ",".to_string())); index += 1; continue; } let (end, replacement) = if cte_macros.contains(&index) { - (index, format!("{sentinel} AS (SELECT 1)")) + (index, format!("{} AS (SELECT 1)", sentinel())) } else if !expressions { index += 1; continue; - } else if let Some((_, end)) = cast_types.iter().find(|(start, _)| *start == index) { - (*end, sentinel) + } else if let Some(end) = cast_type_ends.get(&index) { + (*end, sentinel()) } else if token.token_type == TokenType::DColon { ( postfix_type_end(tokens, index + 1)?, - format!("::{sentinel}"), + format!("::{}", sentinel()), ) } else if token.token_type == TokenType::Colon && index > 0 @@ -144,7 +160,7 @@ pub(super) fn protect_syntax( ) }) { - (index + 1, format!(".{sentinel}")) + (index + 1, format!(".{}", sentinel())) } else if is_typed_literal(tokens, index) || (matches!(token.token_type, TokenType::Var | TokenType::Identifier) && tokens.get(index + 1).is_some_and(|next| { @@ -154,12 +170,14 @@ pub(super) fn protect_syntax( .get(index + 2) .is_some_and(|next| next.token_type == TokenType::Arrow)) { - (index + 1, sentinel) - } else if matches!(token.text.to_ascii_uppercase().as_str(), "MOD" | "POSITION") + (index + 1, sentinel()) + } else if (token.text.eq_ignore_ascii_case("MOD") + || token.text.eq_ignore_ascii_case("POSITION")) && tokens .get(index + 1) .is_some_and(|next| next.token_type == TokenType::LParen) { + let sentinel: String = sentinel(); if token.text.eq_ignore_ascii_case("POSITION") { let separator = (index + 2..tokens.len()) .take_while(|&position| { @@ -180,7 +198,7 @@ pub(super) fn protect_syntax( token.token_type, TokenType::String | TokenType::DollarString ) { - (index, format!("'{sentinel}'")) + (index, format!("'{}'", sentinel())) } else { index += 1; continue; @@ -215,6 +233,7 @@ pub(super) fn protect_syntax( prepared.extend(&chars[cursor..]); Ok(ProtectedSql { sql: prepared, + prefix, fragments, in_functions, }) @@ -263,7 +282,7 @@ pub(super) fn cte_macro_indices(tokens: &[Token]) -> HashSet { scopes.insert(depths[index]); } else if token.token_type == TokenType::Select { scopes.remove(&depths[index]); - } else if token.text.starts_with("__sqb_lint_") + } else if token.text.starts_with(LINT_SENTINEL_PREFIX) && scopes.contains(&depths[index]) && index > 0 && matches!( diff --git a/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs b/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs index 80d196c96e..82971be7fa 100644 --- a/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs +++ b/crates/sqlbuild-rules/src/sql_lint/_helpers/token_layout.rs @@ -39,7 +39,7 @@ struct Lexeme { start: usize, end: usize, raw: String, - key: String, + key: LexemeKey, /// In the reserved-keyword and built-in fold set shared with the change fingerprint. foldable: bool, /// An unquoted authored word outside the name positions that keep reserved words exact. @@ -48,6 +48,54 @@ struct Lexeme { keyword: bool, } +/// How lexemes compare: words ignoring ASCII case, punctuation by type, the rest by raw text. +#[derive(Clone, Copy, PartialEq, Eq)] +enum LexemeKey { + Word, + Punctuation(TokenType), + Raw, +} + +impl Lexeme { + fn same_key(&self, other: &Lexeme) -> bool { + self.key == other.key + && match self.key { + LexemeKey::Word => self.raw.eq_ignore_ascii_case(&other.raw), + LexemeKey::Punctuation(_) => true, + LexemeKey::Raw => self.raw == other.raw, + } + } +} + +/// The source text and case-fold flag of every token, read once for the token check. +pub(crate) struct TokenTexts { + texts: Vec, + foldable: Vec, +} + +impl TokenTexts { + #[cfg(test)] + pub(crate) fn read(sql: &str, tokens: &[Token], dialect: &Dialect) -> Result { + let texts = token_texts(sql, tokens)?; + let foldable = foldable_tokens(&texts, tokens, dialect); + Ok(Self { texts, foldable }) + } + + /// Whether token `index` here, upper-cased when foldable, spells `canonical`. + fn same_canonical(&self, index: usize, canonical: &str) -> bool { + let (text, fold) = (&self.texts[index], self.foldable[index]); + text.len() == canonical.len() + && text.bytes().zip(canonical.bytes()).all(|(byte, expected)| { + expected + == if fold { + byte.to_ascii_uppercase() + } else { + byte + } + }) + } +} + /// An authored token the printer recased to the layout guide's keyword spelling. pub(crate) struct KeywordRecase { pub(crate) generated: String, @@ -179,7 +227,7 @@ pub(crate) fn print_authored_tokens( source: &AuthoredSql<'_>, oracle: &OracleLayout, dialect: &Dialect, -) -> Result<(String, BTreeMap), String> { +) -> Result<(String, BTreeMap, TokenTexts), String> { let AuthoredSql { sql, tokens, @@ -246,7 +294,7 @@ pub(crate) fn print_authored_tokens( } else { separators .get(index + 1) - .is_some_and(|next| next.starts_with('\n')) + .is_none_or(|next| next.starts_with('\n')) }; if !next_starts_line { output.push('\n'); @@ -256,29 +304,42 @@ pub(crate) fn print_authored_tokens( } } } - Ok((output, recases)) + let mut texts = TokenTexts { + texts: Vec::with_capacity(authored.len()), + foldable: Vec::with_capacity(authored.len()), + }; + for lexeme in authored { + texts.texts.push(lexeme.raw); + texts.foldable.push(lexeme.foldable); + } + Ok((output, recases, texts)) } /// Refuse output that changes anything but layout, fold-set case, and recorded keyword recases. -pub(crate) fn verify_token_invariant( - (before, before_tokens): (&str, &[Token]), +pub(crate) fn verify_printed_tokens( + (before, before_tokens, before_texts): (&str, &[Token], &TokenTexts), (after, after_tokens): (&str, &[Token]), dialect: &Dialect, recases: &BTreeMap, ) -> Result<(), String> { - let before_canonical = canonical_tokens(before, before_tokens, dialect)?; let after_canonical = canonical_tokens(after, after_tokens, dialect)?; - if before_canonical.len() != after_canonical.len() { + if before_texts.texts.len() != after_canonical.len() { return Err(TOKEN_PRESERVATION_FAILURE.to_string()); } - let before_texts = token_texts(before, before_tokens)?; - let after_texts = token_texts(after, after_tokens)?; - for (index, (left, right)) in before_canonical.iter().zip(&after_canonical).enumerate() { - if left != right - && !recases - .get(&index) - .is_some_and(|recase| recase.allows(&before_texts[index], &after_texts[index])) - { + let mut after_texts: Option> = None; + for (index, canonical) in after_canonical.iter().enumerate() { + if before_texts.same_canonical(index, &canonical.text) { + continue; + } + let Some(recase) = recases.get(&index) else { + return Err(TOKEN_PRESERVATION_FAILURE.to_string()); + }; + let texts: &Vec = match after_texts { + Some(ref texts) => texts, + None => after_texts.insert(token_texts(after, after_tokens)?), + }; + let printed: &str = &texts[index]; + if !recase.allows(&before_texts.texts[index], printed) { return Err(TOKEN_PRESERVATION_FAILURE.to_string()); } } @@ -339,17 +400,18 @@ fn lexemes(sql: &str, tokens: &[Token], dialect: Option<&Dialect>) -> Result Lexeme { - let key = if is_unquoted_word(&raw) { - format!("w:{}", raw.to_ascii_uppercase()) + let word = is_unquoted_word(&raw); + let key = if word { + LexemeKey::Word } else if raw .chars() .all(|character| character.is_ascii_punctuation()) { - format!("p:{:?}", token.token_type) + LexemeKey::Punctuation(token.token_type) } else { - format!("r:{raw}") + LexemeKey::Raw }; - let keyword = is_unquoted_word(&raw) + let keyword = word && !matches!( token.token_type, TokenType::Var | TokenType::Identifier | TokenType::QuotedIdentifier @@ -433,7 +495,7 @@ struct Alignment<'a> { impl Alignment<'_> { fn matches(&self, left: usize, right: usize) -> bool { - if self.authored[left].key != self.generated[right].key { + if !self.authored[left].same_key(&self.generated[right]) { return false; } if !is_closer(&self.authored[left].raw) { @@ -468,7 +530,7 @@ impl Alignment<'_> { continue; } let anchor = &self.authored[left + skip_left]; - let confirmations = if anchor.key.starts_with("p:") { + let confirmations = if matches!(anchor.key, LexemeKey::Punctuation(_)) { RESYNC_CONFIRMATION } else { 0 @@ -478,7 +540,7 @@ impl Alignment<'_> { self.authored.get(left + skip_left + offset), self.generated.get(right + skip_right + offset), ) { - (Some(before), Some(after)) => before.key == after.key, + (Some(before), Some(after)) => before.same_key(after), _ => true, } }); @@ -569,7 +631,7 @@ fn separators( separators[next] = if substitution { let oracle_gap = (previous_at + 1..next_at) .rev() - .find(|&at| generated[at].key == authored[last].key) + .find(|&at| generated[at].same_key(&authored[last])) .map_or_else(|| gap(next_at - 1, next_at), |at| gap(at, at + 1)); let unspaced = default_space(&authored[last], &authored[next]).is_empty(); if unspaced && !oracle_gap.contains('\n') { diff --git a/crates/sqlbuild-rules/src/sql_lint/tests.rs b/crates/sqlbuild-rules/src/sql_lint/tests.rs index 5e5d936598..d1ff27056d 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests.rs @@ -14,6 +14,8 @@ mod format_corpus; mod function_call_spacing; #[path = "tests/helpers.rs"] mod helpers; +#[path = "tests/test_idempotency_reprint.rs"] +mod idempotency_reprint; #[path = "tests/test_layout_rules.rs"] mod layout_rules; #[path = "tests/test_line_wrap.rs"] diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs index af36b480a1..cb29e9c288 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_authored_tokens.rs @@ -2,7 +2,9 @@ use serde_json::{Value, json}; use std::collections::BTreeMap; -use crate::sql_lint::_helpers::token_layout::{KeywordRecase, tokenize, verify_token_invariant}; +use crate::sql_lint::_helpers::token_layout::{ + KeywordRecase, TokenTexts, tokenize, verify_printed_tokens, +}; use crate::sql_lint::main::formatter::format_json; use crate::sql_lint::tests::test_types; @@ -396,8 +398,10 @@ fn given_formatted_candidates_when_checking_token_invariant_then_only_layout_may .collect(); let before_tokens = tokenize(test_case.before, &dialect).expect("before tokenizes"); let after_tokens = tokenize(test_case.after, &dialect).expect("after tokenizes"); - let actual = verify_token_invariant( - (test_case.before, &before_tokens), + let before_texts = + TokenTexts::read(test_case.before, &before_tokens, &dialect).expect("before reads"); + let actual = verify_printed_tokens( + (test_case.before, &before_tokens, &before_texts), (test_case.after, &after_tokens), &dialect, &recases, diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_comment_formatter.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_comment_formatter.rs index 5519b0a18f..adc81ce1ed 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests/test_comment_formatter.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_comment_formatter.rs @@ -51,3 +51,45 @@ fn given_leading_cte_comments_when_formatting_then_comments_stay_above_their_cte } Ok(()) } + +#[test] +fn given_trailing_comments_after_the_last_token_when_formatting_then_no_newline_is_added() +-> Result<(), String> { + let test_cases = [ + test_types::FormatTestCase { + description: "a trailing line comment ends the output without a newline", + sql: "SELECT 1 AS order_id\nFROM orders -- latest orders", + expected_sql: "SELECT 1 AS order_id\nFROM orders -- latest orders", + expected_changed: false, + }, + test_types::FormatTestCase { + description: "line comments after the last token keep no trailing newline", + sql: "select 1 as order_id from orders\n\n-- first note\n\n-- second note", + expected_sql: "SELECT 1 AS order_id\nFROM orders -- first note\n -- second note", + expected_changed: true, + }, + test_types::FormatTestCase { + description: "a trailing block comment ends the output", + sql: "SELECT 1 AS order_id\nFROM orders /* latest orders */", + expected_sql: "SELECT 1 AS order_id\nFROM orders /* latest orders */", + expected_changed: false, + }, + ]; + for test_case in &test_cases { + let response = format_json( + &json!({"version": 1, "sql": test_case.sql, "dialect": "duckdb"}).to_string(), + )?; + let payload: Value = serde_json::from_str(&response).map_err(|error| error.to_string())?; + assert_eq!( + payload["sql"], test_case.expected_sql, + "{}", + test_case.description + ); + assert_eq!( + payload["changed"], test_case.expected_changed, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_idempotency_reprint.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_idempotency_reprint.rs new file mode 100644 index 0000000000..edf70b5826 --- /dev/null +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_idempotency_reprint.rs @@ -0,0 +1,48 @@ +use serde_json::{Value, json}; + +use crate::sql_lint::_helpers::formatter::RELAYOUTS; +use crate::sql_lint::main::formatter::format_json; +use crate::sql_lint::tests::test_types; + +#[test] +fn given_formatted_and_unformatted_bodies_when_formatting_then_only_changed_bodies_are_reprinted() +-> Result<(), String> { + let test_cases = [ + test_types::ReprintTestCase { + description: "an already formatted body is not re-printed", + sql: "SELECT\n order_id,\n amount\nFROM orders\nWHERE amount > 0", + expected_changed: false, + expected_reprints: 0, + }, + test_types::ReprintTestCase { + description: "a body the formatter changes is re-printed once", + sql: "select order_id, amount from orders where amount > 0", + expected_changed: true, + expected_reprints: 1, + }, + ]; + for test_case in &test_cases { + RELAYOUTS.with(|relayouts| relayouts.set(0)); + let response = format_json( + &json!({"version": 1, "sql": test_case.sql, "dialect": "duckdb"}).to_string(), + )?; + let payload: Value = serde_json::from_str(&response).map_err(|error| error.to_string())?; + assert_eq!( + payload["formatted"], true, + "{}: {payload}", + test_case.description + ); + assert_eq!( + payload["changed"], test_case.expected_changed, + "{}: {payload}", + test_case.description + ); + assert_eq!( + RELAYOUTS.with(std::cell::Cell::get), + test_case.expected_reprints, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_sql_lint.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_sql_lint.rs index 2c8bcbbde8..ab2d111445 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests/test_sql_lint.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_sql_lint.rs @@ -1890,7 +1890,7 @@ fn given_format_cases_when_formatting_then_output_matches() -> Result<(), String test_types::FormatTestCase { description: "trailing comment after a kept statement terminator", sql: "select a from items; -- retained", - expected_sql: "SELECT a\nFROM items; -- retained\n", + expected_sql: "SELECT a\nFROM items; -- retained", expected_changed: true, }, test_types::FormatTestCase { diff --git a/crates/sqlbuild-rules/src/sql_lint/tests/test_types.rs b/crates/sqlbuild-rules/src/sql_lint/tests/test_types.rs index d036ebdecc..30f7edda4a 100644 --- a/crates/sqlbuild-rules/src/sql_lint/tests/test_types.rs +++ b/crates/sqlbuild-rules/src/sql_lint/tests/test_types.rs @@ -132,6 +132,13 @@ pub(crate) struct FormatTestCase { pub expected_changed: bool, } +pub(crate) struct ReprintTestCase { + pub description: &'static str, + pub sql: &'static str, + pub expected_changed: bool, + pub expected_reprints: usize, +} + pub(crate) struct InvalidRuleTestCase { pub description: &'static str, pub rule: &'static str, diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 2499250938..1a30877064 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -315,6 +315,15 @@ class FormatCompileIntegrationTestCase: expected_literal: str +@dataclass(frozen=True) +class TrailingCommentFormatIntegrationTestCase: + """A model body ending in comments that formatting must settle on in one run.""" + + description: str + body: str + expected_body: str + + @dataclass(frozen=True) class AuthoredSpellingFormatIntegrationTestCase: """One authored function or operator spelling that formatting must keep.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py index 398aa37bb7..f56f66e489 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py @@ -26,6 +26,7 @@ LeadingCteCommentFormatIntegrationTestCase, LineWidthWrapIntegrationTestCase, MixedFromValuesFormatIntegrationTestCase, + TrailingCommentFormatIntegrationTestCase, TypedNullFormatIntegrationTestCase, ) from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( @@ -436,6 +437,44 @@ def test_given_description_width_when_formatting_then_wrapped_model_compiles( ) +@pytest.mark.parametrize( + "test_case", + [ + TrailingCommentFormatIntegrationTestCase( + description="a trailing line comment on the last line stays settled", + body="SELECT 1 AS order_id\nFROM orders -- latest orders\n", + expected_body="SELECT 1 AS order_id\nFROM orders -- latest orders\n", + ), + TrailingCommentFormatIntegrationTestCase( + description="comments after the last clause gain no blank line", + body="select 1 as order_id from orders\n\n-- first note\n\n-- second note\n", + expected_body="SELECT 1 AS order_id\nFROM orders -- first note\n -- second note\n", + ), + ], + ids=lambda case: case.description, +) +def test_given_trailing_comments_when_formatting_twice_then_second_run_changes_nothing( + test_case: TrailingCommentFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + header: str = 'MODEL (description "Orders placed by customers.");\n\n' + model: Path = tmp_path / "models" / "orders.sql" + model.parent.mkdir() + model.write_text(header + test_case.body, encoding="utf-8") + + first_exit: int = main(["--project-dir", str(tmp_path), "format", "--no-cache"]) + formatted_once: str = model.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(tmp_path), "format", "--no-cache"]) + check_exit: int = main(["--project-dir", str(tmp_path), "format", "--check", "--no-cache"]) + + assert (first_exit, second_exit, check_exit) == (0, 0, 0) + assert formatted_once == header + test_case.expected_body + assert model.read_text(encoding="utf-8") == formatted_once + + @pytest.mark.parametrize( "test_case", [