Skip to content

Comments

Adds a --quote-all option to the command line#179

Closed
Adam-Mustafa wants to merge 2 commits intopgplex:mainfrom
Adam-Mustafa:add-option-quote-all
Closed

Adds a --quote-all option to the command line#179
Adam-Mustafa wants to merge 2 commits intopgplex:mainfrom
Adam-Mustafa:add-option-quote-all

Conversation

@Adam-Mustafa
Copy link
Contributor

Adds a --quote-all option to the command line which will make all column names be wrapped in quotes, not just the ones that need to be.

Copilot AI review requested due to automatic review settings November 27, 2025 19:26
Copy link
Contributor

Copilot AI left a 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 PR adds a --quote-all command-line flag that forces all identifiers (table names, column names, schemas) to be wrapped in double quotes, regardless of whether they are PostgreSQL reserved words or contain special characters requiring quotation.

Key changes:

  • Introduces new quoting functions that accept a forceQuote boolean parameter
  • Propagates the --quote-all flag through the command execution pipeline
  • Adds comprehensive test coverage for the new quoting behavior

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
testdata/dump/quote_all_test/pgdump.sql Test fixture with mixed identifier types to validate quote-all behavior
ir/quote_test.go Unit tests for new force-quoting functions
ir/quote.go New functions QuoteIdentifierWithForce and QualifyEntityNameWithQuotesAndForce
internal/diff/table.go Updated table/column SQL generation to use force-quoting when enabled
internal/diff/diff.go Added Option pattern for migration configuration and QuoteAll option
cmd/root.go Added --quote-all persistent flag and accessor function
cmd/plan/plan.go Retrieves and passes quote-all flag to plan generation
cmd/dump/dump_integration_test.go Integration test validating quote-all behavior
cmd/dump/dump.go Retrieves and passes quote-all flag to dump execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Adam-Mustafa Adam-Mustafa force-pushed the add-option-quote-all branch 2 times, most recently from e170c3a to b062113 Compare November 27, 2025 19:39
Copy link
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem are we trying to solve here? We'd like to avoid adding flags unless absolute necessary

@Adam-Mustafa
Copy link
Contributor Author

Hi @tianzhou, It's just a choice in the output format. Other tools I've seen quote all values and my concern is that some missing value in the future would cause a break because the keyword is not in the list. At the same time, I didn't want to dramatically change the expected output. So I figured a reasonable middle ground was adding the option as a flag. If that's not in line with the repo, it's not absolutely necessary. It just makes the existing output slightly more brittle but still well within reason.

@tianzhou
Copy link
Contributor

@Adam-Mustafa thanks for providing the background. For the moment, we are holding off adding such a flag.

@tianzhou tianzhou closed this Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants