Improve SQL formatting for CREATE TABLE statements #218
Conversation
… CREATE TABLE statements
WalkthroughRefactors SQL printing and parsing to add context-aware spacing for CREATE TABLE/constraint parentheses, introduce a constraintStyle option (postgres/mysql), thread it through formatter/parser, adjust SqlPrinter signatures/types, and update tests and docs to reflect new formatting behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Formatter as SqlFormatter
participant Parser as SqlPrintTokenParser
participant Printer as SqlPrinter
Formatter->>Parser: parse AST tokens (with constraintStyle)
Parser-->>Formatter: token stream (tokens include constraint/context metadata)
Formatter->>Printer: format tokens (iterates inner tokens)
loop per token
Printer->>Printer: findPreviousSignificantToken(prevToken, priorToken)
Printer->>Printer: shouldSkipSpaceBeforeParenthesis(token, prevSignificant)
alt skip space
Printer-->>Formatter: emit token without space then "("
else default
Printer-->>Formatter: emit token + space then "("
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts (2)
5-41: Solid coverage for CREATE TABLE spacing.Tests exercise table name, CHECK/UNIQUE/PRIMARY KEY spacing. Looks correct and uses SqlFormatter per guidelines. Consider adding a MySQL-style “UNIQUE KEY (col)” case if that dialect is in scope.
42-72: References spacing case is valuable; add one with quoted identifiers.This asserts removal of the space in REFERENCES t (c). Consider a companion with quoted, multi-part names (e.g., references "s"."t"("c")) to guard tokenizer variations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/transformers/SqlPrinter.ts(5 hunks)packages/core/tests/parsers/CreateTableParser.test.ts(1 hunks)packages/core/tests/parsers/DDLParsers.test.ts(1 hunks)packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Always compile and fix TypeScript errors before running tests
Source code comments must be written in English only
For model-driven mappings (mapping.typeInfo and mapping.structure), use convertModelDrivenMapping
Use imports from 'rawsql-ts' rather than local relative paths for library usage
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/tests/parsers/CreateTableParser.test.tspackages/core/src/transformers/SqlPrinter.ts
packages/core/**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Tests are specs — never change expected values without user consultation
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/tests/parsers/CreateTableParser.test.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Remove console.log statements before commit
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/tests/parsers/CreateTableParser.test.tspackages/core/src/transformers/SqlPrinter.ts
packages/core/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Use SqlFormatter for SQL comparison tests
Files:
packages/core/tests/parsers/DDLParsers.test.tspackages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/tests/parsers/CreateTableParser.test.ts
🧠 Learnings (1)
📚 Learning: 2025-10-18T02:29:34.437Z
Learnt from: CR
PR: mk3008/rawsql-ts#0
File: packages/core/AGENTS.md:0-0
Timestamp: 2025-10-18T02:29:34.437Z
Learning: Applies to packages/core/**/*.{test,spec}.{ts,tsx} : Use SqlFormatter for SQL comparison tests
Applied to files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/tests/parsers/CreateTableParser.test.ts
🧬 Code graph analysis (2)
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts (1)
packages/core/src/parsers/CreateTableParser.ts (1)
CreateTableParser(48-714)
packages/core/src/transformers/SqlPrinter.ts (1)
packages/core/src/models/SqlPrintToken.ts (1)
SqlPrintToken(111-141)
🔇 Additional comments (3)
packages/core/tests/parsers/CreateTableParser.test.ts (1)
94-96: Expectation update aligns with new spacing rule.The assertion for FK formatting now correctly expects no space before parentheses. Looks good and matches the PR objective. Please confirm this behavior is intended across all supported dialects (e.g., Postgres, MySQL) so we can note it as a formatting change in release notes.
packages/core/tests/parsers/DDLParsers.test.ts (1)
84-86: Unique constraint spacing matches new style.Expectation updated to unique("email"). This aligns with the PR’s spacing rule. Please confirm we also want this for MySQL’s UNIQUE KEY form, if supported.
packages/core/src/transformers/SqlPrinter.ts (1)
323-336: Propagation of previous significant token for space handling looks good.Capturing previousChild and threading it into handleSpaceToken enables the targeted skip. This is the right place in the hot path and stays scoped to container contexts. LGTM.
If there are top-level space tokens not covered by the inner loop, consider computing a sibling-aware previous at that callsite in the future, but not required for this PR.Also applies to: 664-694
…ement MySQL-style constraint name handling
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/parsers/SqlPrintTokenParser.ts (1)
2826-2863: Remove 'primary-key' from inlineNameKinds to fix MySQL named PRIMARY KEY syntaxMySQL syntax for named PRIMARY KEY constraints is
CONSTRAINT <name> PRIMARY KEY (...), notPRIMARY KEY <name>(...). The current code generates invalid SQL by inlining names for PRIMARY KEY. UNIQUE KEY and FOREIGN KEY do support inline naming in MySQL and should remain in the set.Apply this fix:
- const inlineNameKinds = new Set<TableConstraintDefinition['kind']>(['primary-key', 'unique', 'foreign-key']); + // MySQL allows inline names for UNIQUE KEY and (index name after) FOREIGN KEY, but not after PRIMARY KEY. + const inlineNameKinds = new Set<TableConstraintDefinition['kind']>(['unique', 'foreign-key']);Also add a unit test covering named PRIMARY KEY with MySQL constraint style, as the test suite only covers UNIQUE KEY and FOREIGN KEY inline forms for MySQL.
🧹 Nitpick comments (6)
packages/core/src/parsers/SqlPrintTokenParser.ts (1)
87-101: Preset consistency: consider making constraintStyle explicit for all presetsYou rely on a global default ('postgres') when a preset omits constraintStyle. Explicitly setting constraintStyle for every PRESETS entry improves predictability and avoids regressions if the default changes later.
Also applies to: 108-131
docs/guide/formatting-recipes.md (1)
136-151: Clarify MySQL naming nuances for PRIMARY/FOREIGN KEYAdd a short note:
- PRIMARY KEY: name via “CONSTRAINT PRIMARY KEY (...)” (not “PRIMARY KEY (...)”).
- FOREIGN KEY: inline name after “FOREIGN KEY” is the index name; the constraint symbol uses the CONSTRAINT prefix.
This prevents users from assuming all three kinds accept the same inline form.
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts (1)
74-106: Add guard test: named PRIMARY KEY under MySQL style (no inline name)To prevent regressions, add a test where input has a named PK (via CONSTRAINT), and verify output remains “constraint primary key(...)” under constraintStyle: 'mysql' (i.e., no “primary key (...)”).
packages/core/src/parsers/CreateTableParser.ts (2)
489-493: Prefer explicit comment: inlineKeyName precedenceMinor: add a comment that inlineKeyName is used only when no prefixed CONSTRAINT name is present to make intent obvious.
496-505: Foreign key inline token can be index name, not constraint symbolIn MySQL, the identifier after “FOREIGN KEY” is the index name; the constraint symbol is provided via “CONSTRAINT ...”. Since you store inlineKeyName into constraintName, please ensure downstream consumers don’t assume it’s always the constraint symbol (e.g., for DROP operations). If necessary, carry a separate indexName field in the model.
packages/core/src/transformers/SqlFormatter.ts (1)
8-11: Avoid preset name drift — derive PresetName from PRESETSHard-coding VALID_PRESETS risks diverging from PRESETS keys (you already have many more). Derive the type instead.
Apply this diff:
-export const VALID_PRESETS = ['mysql', 'postgres', 'sqlserver', 'sqlite'] as const; -export type PresetName = (typeof VALID_PRESETS)[number]; +export type PresetName = keyof typeof PRESETS;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/guide/formatting-recipes.md(2 hunks)packages/core/src/parsers/CreateTableParser.ts(2 hunks)packages/core/src/parsers/SqlPrintTokenParser.ts(6 hunks)packages/core/src/transformers/SqlFormatter.ts(3 hunks)packages/core/src/transformers/SqlPrinter.ts(5 hunks)packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/**/*.{ts,tsx}: Always compile and fix TypeScript errors before running tests
Source code comments must be written in English only
For model-driven mappings (mapping.typeInfo and mapping.structure), use convertModelDrivenMapping
Use imports from 'rawsql-ts' rather than local relative paths for library usage
Files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/src/transformers/SqlFormatter.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/transformers/SqlPrinter.tspackages/core/src/parsers/SqlPrintTokenParser.ts
packages/core/**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Tests are specs — never change expected values without user consultation
Files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts
packages/core/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Remove console.log statements before commit
Files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.tspackages/core/src/transformers/SqlFormatter.tspackages/core/src/parsers/CreateTableParser.tspackages/core/src/transformers/SqlPrinter.tspackages/core/src/parsers/SqlPrintTokenParser.ts
packages/core/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Use SqlFormatter for SQL comparison tests
Files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts
🧠 Learnings (1)
📚 Learning: 2025-10-18T02:29:34.437Z
Learnt from: CR
PR: mk3008/rawsql-ts#0
File: packages/core/AGENTS.md:0-0
Timestamp: 2025-10-18T02:29:34.437Z
Learning: Applies to packages/core/**/*.{test,spec}.{ts,tsx} : Use SqlFormatter for SQL comparison tests
Applied to files:
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts
🧬 Code graph analysis (5)
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts (2)
packages/core/src/parsers/CreateTableParser.ts (1)
CreateTableParser(48-730)packages/core/src/transformers/SqlFormatter.ts (1)
SqlFormatter(118-178)
packages/core/src/transformers/SqlFormatter.ts (1)
packages/core/src/parsers/SqlPrintTokenParser.ts (2)
ConstraintStyle(69-69)SqlPrintTokenParser(201-3175)
packages/core/src/parsers/CreateTableParser.ts (2)
packages/core/src/models/ValueComponent.ts (1)
IdentifierString(277-284)packages/core/src/models/CreateTableQuery.ts (1)
TableConstraintDefinition(101-132)
packages/core/src/transformers/SqlPrinter.ts (1)
packages/core/src/models/SqlPrintToken.ts (1)
SqlPrintToken(111-141)
packages/core/src/parsers/SqlPrintTokenParser.ts (1)
packages/core/src/models/CreateTableQuery.ts (1)
TableConstraintDefinition(101-132)
🔇 Additional comments (12)
packages/core/src/parsers/SqlPrintTokenParser.ts (1)
69-85: Good public-surface additionAdding ConstraintStyle and threading it into FormatterConfig is clean and backwards compatible. LGTM.
docs/guide/formatting-recipes.md (1)
41-41: Docs option row LGTMClear description and defaults for constraintStyle.
packages/core/tests/transformers/SqlFormatter.create-table-spacing.test.ts (2)
5-40: Solid coverage for no-space-before '('Good end-to-end assertion using SqlFormatter; expectations match the PR goal.
42-72: FK spacing case reads wellThe REFERENCES branch formatting looks correct and readable.
packages/core/src/parsers/CreateTableParser.ts (1)
474-483: Inline name capture for UNIQUE KEY is sensibleParsing an inline name between the keyword and '(' aligns with MySQL syntax and helps formatting.
packages/core/src/transformers/SqlFormatter.ts (2)
1-1: Plumbing constraintStyle through the formatter: LGTMImport and option wiring are correct and default to 'postgres' safely.
141-153: Default resolution for constraintStyle is clearMerging option, preset, and fallback is straightforward. No changes needed.
packages/core/src/transformers/SqlPrinter.ts (5)
326-338: LGTM: Token tracking logic is correct.The new logic correctly finds the previous and prior significant tokens by skipping whitespace and comments, then passes them to
handleSpaceTokenfor context-aware spacing decisions. This enables the formatter to make informed decisions about CREATE TABLE spacing.
671-695: LGTM: Space handling correctly implements CREATE TABLE spacing logic.The updated
handleSpaceTokensignature accepts additional context tokens to enable precise spacing decisions. The early return at lines 692-695 correctly skips spaces before structural parentheses in CREATE TABLE DDL, as intended by the PR objectives.
699-711: LGTM: Token filtering logic is sound.The
findPreviousSignificantTokenmethod correctly skips whitespace and comments to find semantically significant tokens. Returning both the token and its index enables efficient multi-step lookback for prior context.
713-811: LGTM: CREATE TABLE spacing logic correctly implements context-aware formatting.The new helper methods implement comprehensive context-aware spacing for CREATE TABLE statements:
shouldSkipSpaceBeforeParenthesis (713-744): Correctly identifies three cases where spaces should be omitted before '(':
- After table name:
CREATE TABLE name(...)- After constraint keywords:
PRIMARY KEY(...)- After identifiers following constraint keywords:
REFERENCES table(...)isCreateTableSpacingContext (746-757): Appropriately scopes the logic to relevant container types (CreateTableQuery, CreateTableDefinition, constraint definitions, and ReferenceDefinition).
isCreateTableConstraintKeyword (767-785): Special handling for ReferenceDefinition (lines 773-775) ensures 'references' keyword is recognized, addressing the past review comment about REFERENCES spacing.
isIdentifierAttachedToConstraint (787-811): Correctly handles both qualified table names after REFERENCES (lines 796-799) and inline constraint names.
The implementation addresses both past review comments:
- "unique key" is now in CREATE_TABLE_MULTI_PAREN_KEYWORDS (line 12)
- REFERENCES table name spacing is handled via ReferenceDefinition context checking (lines 773-775, 796-799)
11-13: Great! I've confirmed that KEY is a synonym for INDEX in MySQL, and the syntax KEYidx_name(column) is valid in CREATE TABLE. This validates adding both 'key' and 'index' toCREATE_TABLE_SINGLE_PAREN_KEYWORDS.Now let me verify test coverage comprehensively:
Based on my verification through web searches and test file analysis:
The 'key' and 'index' keywords are valid and correctly implemented.
In MySQL, KEY is normally a synonym for INDEX and can be used interchangeably. The test file contains explicit test coverage for these cases—specifically the MySQL constraint test (lines 73–102) which validates
UNIQUE KEY uk(id)formatting, confirming that the keyword handling is correct.The standalone 'key' keyword is documented in MySQL as valid syntax for defining indexes in CREATE TABLE (e.g.,
KEY idx_name (col)), and the constants on lines 11–13 appropriately capture this. The context-limiting logic at line 722 (isCreateTableSpacingContext) ensures these keywords only match in CREATE TABLE DDL, preventing false positives.
Omit spaces before parentheses in CREATE TABLE statements to enhance SQL formatting consistency. This change applies to table names and constraint keywords.
Summary by CodeRabbit
New Features
Style
unique(...),foreign key(...)) for consistent output.Tests
Documentation