-
Notifications
You must be signed in to change notification settings - Fork 50
Add line break and indentation for SETTINGS clause in beautify mode #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Modified SettingsClause.FormatSQL() to add proper indentation: - SETTINGS keyword now appears on its own line - Settings items are indented with proper line breaks between them - Updated test fixtures to reflect the new formatting Co-authored-by: git-hulk <4987594+git-hulk@users.noreply.github.com>
Changed WriteByte(',') instead of WriteString(", ") to avoid trailing
spaces after commas in beautified SETTINGS output. Updated all test
fixtures accordingly.
Co-authored-by: git-hulk <4987594+git-hulk@users.noreply.github.com>
|
@copilot Resolve conflicts using |
I ran |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modifies the SQL beautifier to format the SETTINGS clause on a new line with proper indentation, making it consistent with other multi-line clauses like SELECT items, ORDER BY, and WITH.
Changes:
- Modified
SettingsClause.FormatSQL()to use theIndent()/Dedent()pattern withBreak()calls between items - Changed comma separator from
", "to,to eliminate trailing whitespace before line breaks - Updated 14 test files to reflect the new SETTINGS formatting behavior
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| parser/format.go | Core formatting change to SettingsClause.FormatSQL() implementing multi-line formatting with proper indentation |
| parser/testdata/query/format/beautify/select_with_settings_additional_table_filters.sql | Updated expected output for SELECT statements with SETTINGS clause |
| parser/testdata/ddl/format/beautify/create_with_time_zone.sql | Updated expected output for CREATE TABLE with multiple settings |
| parser/testdata/ddl/format/beautify/create_table_with_tuple_fields.sql | Updated expected output for CREATE TABLE with single setting |
| parser/testdata/ddl/format/beautify/create_table_with_ttl_policy.sql | Updated expected output for CREATE TABLE with multiple settings and TTL |
| parser/testdata/ddl/format/beautify/create_table_with_sample_by.sql | Updated expected output for CREATE TABLE with SAMPLE BY and settings |
| parser/testdata/ddl/format/beautify/create_table_with_nullable.sql | Updated expected output for CREATE TABLE with nullable fields |
| parser/testdata/ddl/format/beautify/create_table_with_keyword_partition_by.sql | Updated expected output for CREATE TABLE with keyword partition |
| parser/testdata/ddl/format/beautify/create_table_with_index.sql | Updated expected output for CREATE TABLE with indexes and multiple settings |
| parser/testdata/ddl/format/beautify/create_table_with_enum_fields.sql | Updated expected output for CREATE TABLE with enum fields |
| parser/testdata/ddl/format/beautify/create_table_with_codec_delta.sql | Updated expected output for CREATE TABLE with codec settings |
| parser/testdata/ddl/format/beautify/create_materialized_view_with_refresh.sql | Updated expected output for CREATE MATERIALIZED VIEW with REFRESH and settings (contains formatting issue) |
| parser/testdata/ddl/format/beautify/create_distributed_table.sql | Updated expected output for CREATE DISTRIBUTED TABLE |
| parser/testdata/ddl/format/beautify/alter_table_drop_detach_partition.sql | Updated expected output for ALTER TABLE with settings |
| parser/testdata/ddl/format/beautify/alter_table_add_column.sql | Updated expected output for ALTER TABLE ADD COLUMN with settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Merge the master branch and resolve conflicts |
- Merged origin/master which added FROM clause indentation - Resolved conflicts in test fixtures using make update_test - Combined both FROM and SETTINGS indentation formatting - All tests passing Co-authored-by: git-hulk <4987594+git-hulk@users.noreply.github.com>
Merged master branch successfully. Conflicts in test fixtures were resolved using |
Added Break() before APPEND when Settings is present to ensure APPEND appears on a new line instead of inline with the last setting item. Addresses code review feedback about inconsistent formatting. Co-authored-by: git-hulk <4987594+git-hulk@users.noreply.github.com>
Pull Request Test Coverage Report for Build 21973856305Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Should be: |
Changed ALTER TABLE operations to use Break() instead of WriteByte(whitespace) before SETTINGS clause, ensuring SETTINGS appears on a new line in beautify mode: - AlterTableAddColumn - AlterTableDetachPartition - AlterTableDropPartition This makes ALTER TABLE SETTINGS formatting consistent with SELECT and other statement types. Co-authored-by: git-hulk <4987594+git-hulk@users.noreply.github.com>
... Fixed in commit 5910b22. Changed ALTER TABLE operations (ADD COLUMN, DETACH PARTITION, DROP PARTITION) to use Break() before SETTINGS, so it now appears on a new line as expected: |
SettingsClause.FormatSQL()to add indentation after SETTINGS keywordOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.