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
|
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: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges
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: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/postgresql.test.ts (1)
237-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for following positive signs.
These assertions exercise only the incoming
-path. Add cases with+operands to verify the separateitem.startsWith('+')branch.Example coverage
+ expect(format('SELECT 5 % +2, 2 ^ +2, 8 # +1', { denseOperators: true })).toBe(dedent` + SELECT + 5% +2, + 2^ +2, + 8# +1 + `);🤖 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 `@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.
🤖 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.
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.
🪄 Autofix (Beta)
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
📒 Files selected for processing (2)
src/formatter/Layout.tstest/postgresql.test.ts
…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.
666fa4e to
36c7e58
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/formatter/Layout.ts (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete
lastItemEndsWithhelper.
Layout.addnow callswouldMergeIntoOperatoron Line 65. No code insrc/formatter/Layout.tscallslastItemEndsWith. Remove the unused private method to avoid stale logic and unused-private-member checks.🤖 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/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.
🤖 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/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
📒 Files selected for processing (2)
src/formatter/Layout.tstest/postgresql.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/postgresql.test.ts
nene
left a comment
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.
| // 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.
| 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().
| 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.
| 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.
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 |
2 participants
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. So5%-2is tokenized as5 %- 2, and%-is not a defined operator:The same happens for
^ # & | ~ ! @and multi-character operators that contain them (for example@>becomes@>-). SodenseOperatorscan 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 like5*-2anda=-1is unchanged.Test
Added to
test/postgresql.test.ts. The full suite passes (5842 tests).