fix: keep a space between an operator and a following sign with denseOperators - #962
fix: keep a space between an operator and a following sign with denseOperators#962spokodev wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 35ea26c2-85c7-4b3e-b4ee-ead5cb4126e7 📥 CommitsReviewing files that changed from the base of the PR and between 36c7e58 and d6926ff. 📒 Files selected for processing (7)
📝 Walkthrough Summary by CodeRabbit
WalkthroughChangesoperatorsCombine is added to dialect format settings and passed to Layout. Layout.add now prevents unsafe merges between trailing operator runs and incoming - or + tokens. PostgreSQL and MySQL tests cover signed operands in dense operator mode. Dense operator formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Dialect
participant Formatter
participant Layout
participant Output
Dialect->>Formatter: provide normalized operatorsCombine
Formatter->>Layout: format statement with operatorsCombine
Layout->>Layout: detect unsafe operator merge
Layout->>Output: emit separated signed operand tokens
Possibly related PRs
Suggested reviewers: nene 🚥 Pre-merge checks | ✅ 4 ✅ Passed checks (4 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)test/postgresql.test.ts (1)🤖 Prompt for all review comments with AI agents237-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for following positive signs.
These assertions exercise only the incoming - path. Add cases with + operands to verify the separate item.startsWith('+') branch.
Example coverage🤖 Prompt for AI Agents+ expect(format('SELECT 5 % +2, 2 ^ +2, 8 # +1', { denseOperators: true })).toBe(dedent` + SELECT + 5% +2, + 2^ +2, + 8# +1 + `);Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/postgresql.test.ts` around lines 237 - 248, Extend the denseOperators test in the “keeps a space between an operator and a following sign” case to include operands prefixed with “+”. Cover both the arithmetic operator examples and the PostgreSQL JSONB operator path so the item.startsWith('+') branch is exercised, while preserving the expected space between each operator and positive signed operand.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/formatter/Layout.ts`:
- Around line 78-92: Update wouldMergeIntoOperator in Layout.ts to include
PostgreSQL’s ? and backtick characters in both the trailing operator run regex
and the sensitive-character test, preserving the existing merge behavior for
operators ending in - and all other guarded operator characters.
---
Nitpick comments:
In `@test/postgresql.test.ts`:
- Around line 237-248: Extend the denseOperators test in the “keeps a space
between an operator and a following sign” case to include operands prefixed with
“+”. Cover both the arithmetic operator examples and the PostgreSQL JSONB
operator path so the item.startsWith('+') branch is exercised, while preserving
the expected space between each operator and positive signed operand.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 73acd2db-976e-4e97-8f05-fb4c262666ad
📥 CommitsReviewing files that changed from the base of the PR and between aa8efae and 666fa4e.
📒 Files selected for processing (2)- src/formatter/Layout.ts
- test/postgresql.test.ts
Sorry, something went wrong.
…Operators A binary operator immediately followed by a unary + or - was glued to it with denseOperators, so 'SELECT 5 % -2' became '5%-2'. PostgreSQL lexes a run of operator characters greedily, so an operator containing one of ~!@#%^&|`? keeps a trailing sign: '%' and '-' merge into a single '%-' operator (which does not exist), '@>' and '-' into '@>-', and the jsonb '?' and '-' into '?-'. The query then errors or changes meaning. Generalize the existing '--' line-comment guard to also keep a space in these cases.
There was a problem hiding this comment.
src/formatter/Layout.ts (1)🤖 Prompt for all review comments with AI agents60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the obsolete lastItemEndsWith helper.
Layout.add now calls wouldMergeIntoOperator on Line 65. No code in src/formatter/Layout.ts calls lastItemEndsWith. Remove the unused private method to avoid stale logic and unused-private-member checks.
🤖 Prompt for AI AgentsVerify 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/Layout.ts` around lines 60 - 65, Remove the unused private lastItemEndsWith helper from Layout; retain the existing wouldMergeIntoOperator call in Layout.add and all other formatting behavior unchanged.
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/Layout.ts`: - Around line 60-65: Remove the unused private lastItemEndsWith helper from Layout; retain the existing wouldMergeIntoOperator call in Layout.add and all other formatting behavior unchanged.
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 180c87d1-3c37-4804-bf15-7f7597e4951d
📥 CommitsReviewing files that changed from the base of the PR and between 666fa4e and 36c7e58.
📒 Files selected for processing (2)- src/formatter/Layout.ts
- test/postgresql.test.ts
- test/postgresql.test.ts
Sorry, something went wrong.
nene
left a comment
There was a problem hiding this comment.
A major problem with this pull request is that it's tackling an issue that's specific to PostgreSQL, but the implementation is written for all dialects.
To my knowledge this operator concatenation is really mainly an issue in PostgreSQL. Most SQL dialects don't support all these fancy operators. It really would be best if this fix was constrained to only target PostgreSQL.
Sorry, something went wrong.
| // Don't glue an item starting with "-"/"+" onto a preceding operator when | ||
| // the two would re-lex as one token: "-" onto "-" forms "--" (a line | ||
| // comment that swallows the rest of the line), and a sign onto an operator | ||
| // containing ~!@#%^&|`? forms a merged operator like "%-" or "@>-" that parses | ||
| // differently (e.g. densing "5 % -2" into "5%-2"). | ||
| if (this.wouldMergeIntoOperator(item)) { |
There was a problem hiding this comment.
This long comment would really be better rewritten as a comment on the wouldMergeIntoOperator() method, describing what that method does.
Sorry, something went wrong.
| return typeof lastItem === 'string' && lastItem.endsWith(suffix); | ||
| } | ||
|
|
||
| private wouldMergeIntoOperator(item: string): boolean { |
There was a problem hiding this comment.
I find the name of this method to be kinda awkward. Definitely this code base doesn't contain any other function names starting with would- prefix.
I'd suggest inverting the boolean return value of this function and naming it something like isItemSafeToAppend().
Sorry, something went wrong.
| const run = /[-+*/<>=~!@#%^&|`?]+$/u.exec(lastItem)?.[0]; | ||
| if (!run) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I have no idea what's going on in here. What does the run variable mean?
I guess it's some sort of run of characters. But that doesn't really help me in understanding its purpose.
Sorry, something went wrong.
| it('keeps a space between an operator and a following sign with denseOperators', () => { | ||
| expect(format('SELECT 5 % -2, 2 ^ -2, 8 # -1', { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| 5% -2, | ||
| 2^ -2, | ||
| 8# -1 | ||
| `); | ||
| expect(format(`SELECT '[1,2]'::jsonb @> -1`, { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| '[1,2]'::jsonb@> -1 | ||
| `); | ||
| expect(format(`SELECT data ? -1 FROM t`, { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| data? -1 | ||
| FROM | ||
| t |
There was a problem hiding this comment.
I counted 17 special characters in the regular expression. In this test we're only checking a few of them.
Sorry, something went wrong.
The guard that keeps a space between an operator and a following +/- sign only prevents a real bug where the target dialect lexes a run of operator characters as a single operator, so 5 % -2 densed to 5%-2 re-parses as the operator %-. That is PostgreSQL and Redshift; MySQL, standard SQL and the rest have fixed operator sets and re-parse 5%-2 as 5 % -2, so the extra space is not needed. Gate the operator-run branch behind a new operatorsCombine dialect option (true for postgresql/redshift). The -- line-comment guard is unchanged and stays universal, so a - -b keeps its space in every dialect.
|
You're right, thanks. Scoped it to the dialects that lex a run of operator characters as a single operator (PostgreSQL and Redshift), where 5 % -2 densed to 5%-2 re-parses as the operator %-. It's behind a new operatorsCombine dialect option; MySQL, standard SQL and the rest keep a fixed operator set, so they now dense 5 % -2 to 5%-2 unchanged (added a MySQL test covering that). The -- line-comment guard is untouched and stays universal, so a - -b keeps its space everywhere. Full suite green. |
Sorry, something went wrong.
Problem
With denseOperators, a binary operator immediately followed by a unary +/- is glued onto it:
PostgreSQL lexes a run of operator characters greedily, and an operator containing one of ~ ! @ # % ^ & | keeps a trailing +/- as part of the operator. So 5%-2 is tokenized as 5 %- 2, and %- is not a defined operator:
The same happens for ^ # & | ~ ! @ and multi-character operators that contain them (for example @> becomes @>-). So denseOperators can turn a valid query into one that fails to parse or changes meaning.
The formatter already keeps a space for the analogous - before - case (which would otherwise form a -- line comment and swallow the rest of the line). This is the same class of problem, so this change generalizes that guard.
Fix
In Layout, when appending an item that starts with +/-, if the preceding item ends in an operator run that either ends in - (the existing -- case) or contains one of ~ ! @ # % ^ & |, keep a space. Operators that do not merge with a following sign (* / + - << >> = < >) are unaffected, so dense output like 5*-2 and a=-1 is unchanged.
Test
Added to test/postgresql.test.ts. The full suite passes (5842 tests).