Emit comma between CTEs in non-compact WITH formatting#7
Emit comma between CTEs in non-compact WITH formatting#7
Conversation
format_with_clause_left closed every CTE with a bare ")", which
produced invalid SQL whenever there were two or more CTEs in a
style that does not use compact CTEs (e.g. dbt). The compact-CTE
branch already inlined "), name AS (" correctly; the non-compact
branch is now position-aware and emits ")," for every CTE except
the last.
Adds a minimal multi-CTE fixture for the dbt style as a regression
test.
📝 WalkthroughWalkthroughThis PR fixes CTE (Common Table Expression) formatting in left-aligned ChangesCTE Delimiter Formatting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/formatter/select.rs (2)
1577-1585: ⚡ Quick winPre-existing dead code block can be simplified.
The
if self.config.compact_ctes && i > 0 / elseblock at Lines 1577–1585 reduces to a single unconditionallines.push(cte_prefix)— both branches pushcte_prefixand the innerif i > 0 && !self.config.compact_ctesguard wraps an empty body. Worth cleaning up while this function is under active change.♻️ Suggested simplification
- if self.config.compact_ctes && i > 0 { - lines.push(cte_prefix); - } else { - if i > 0 && !self.config.compact_ctes { - // Close previous CTE. - // Already handled below. - } - lines.push(cte_prefix); - } + lines.push(cte_prefix);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatter/select.rs` around lines 1577 - 1585, The conditional branch around self.config.compact_ctes and i is dead: both branches call lines.push(cte_prefix) and the nested if contains no body; simplify by removing the outer if/else and the empty inner guard and replace with a single unconditional lines.push(cte_prefix) at that location (keep references to self.config.compact_ctes and i only if still needed elsewhere); locate the code using the symbols self.config.compact_ctes, i, and lines.push(cte_prefix) in select.rs to make the change.
1595-1602: ⚡ Quick winConsider adding multi-CTE fixtures for other
!compact_ctesstyles (Mozilla, GitLab).The fix is config-flag driven and applies to every style where
compact_ctesisfalse, but regression coverage currently only exists fordbt. Mozilla and GitLab share the same code path and could silently regress in future.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatter/select.rs` around lines 1595 - 1602, Tests only cover the dbt style for the !self.config.compact_ctes behavior; add equivalent multi-CTE fixtures for the Mozilla and GitLab styles so the shared code path (the block that pushes "),"/")" based on is_last using self.config.compact_ctes and the ctes iteration) is exercised for those styles. Create fixtures mirroring the existing multi-CTE dbt case but with the Mozilla and GitLab formatting configs, and update the test harness to run the same assertions against the output produced by the code that calls lines.push(..) for non-compact_ctes to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/formatter/select.rs`:
- Around line 1577-1585: The conditional branch around self.config.compact_ctes
and i is dead: both branches call lines.push(cte_prefix) and the nested if
contains no body; simplify by removing the outer if/else and the empty inner
guard and replace with a single unconditional lines.push(cte_prefix) at that
location (keep references to self.config.compact_ctes and i only if still needed
elsewhere); locate the code using the symbols self.config.compact_ctes, i, and
lines.push(cte_prefix) in select.rs to make the change.
- Around line 1595-1602: Tests only cover the dbt style for the
!self.config.compact_ctes behavior; add equivalent multi-CTE fixtures for the
Mozilla and GitLab styles so the shared code path (the block that pushes
"),"/")" based on is_last using self.config.compact_ctes and the ctes iteration)
is exercised for those styles. Create fixtures mirroring the existing multi-CTE
dbt case but with the Mozilla and GitLab formatting configs, and update the test
harness to run the same assertions against the output produced by the code that
calls lines.push(..) for non-compact_ctes to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b4c3f0-2613-4fa6-9dd1-c414ab8bad07
📒 Files selected for processing (4)
src/formatter/select.rstests/fixtures/dbt/select_cte_multi.expectedtests/fixtures/dbt/select_cte_multi.sqltests/fixtures_test.rs
1 participant
Summary
format_with_clause_leftwas unconditionally pushing)after each CTE body, dropping the comma separator between CTEs whenevercompact_cteswas off (most visibly in thedbtstyle).),for non-final CTEs and)for the last, matching the comma handling already present in the compact-CTE branch.dbtstyle as a regression test.Repro (before fix)
Input:
Formatting with
Style::Dbtproduced)\nb as ((no comma) — invalid SQL.Test plan
cargo test(73 pass; newdbt_select_cte_multi)cargo clippy --all-targets -- -D warningscargo fmt --checkSummary by CodeRabbit
Bug Fixes
Tests