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
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.
|
You're right, thanks. Scoped it to the dialects that lex a run of operator characters as a single operator (PostgreSQL and Redshift), where |
|
All four points addressed. Naming. The comment moved off the
Coverage. The test now runs over every character that makes PostgreSQL lex a run as one operator, rather than a sample: That is nine of the ten characters in the inner class. The tenth is the backtick: it is legal in a Also removed 5852 passing, prettier, eslint and |
nene
left a comment
nene
left a comment
There was a problem hiding this comment.
Thanks for the improvements.
This is better now. However I still have several concerns.
| private isItemSafeToAppend(item: string): boolean { | ||
| if (!item.startsWith('-') && !item.startsWith('+')) { | ||
| return true; | ||
| } | ||
| const lastItem = last(this.items); | ||
| return typeof lastItem === 'string' && lastItem.endsWith(suffix); | ||
| if (typeof lastItem !== 'string') { | ||
| return true; | ||
| } | ||
| // The operator characters the new item would be written against. | ||
| const precedingOperatorChars = /[-+*/<>=~!@#%^&|`?]+$/u.exec(lastItem)?.[0]; | ||
| if (!precedingOperatorChars) { | ||
| return true; | ||
| } | ||
| if (item.startsWith('-') && precedingOperatorChars.endsWith('-')) { | ||
| return false; | ||
| } | ||
| return !(this.operatorsCombine && /[~!@#%^&|`?]/u.test(precedingOperatorChars)); | ||
| } |
There was a problem hiding this comment.
I have trouble understanding the logic in this function. The function has a long comment, which might be a sign that a lot of explanation is needed to explain what this code does.
Some observations:
- it starts with check
item.startsWith('+')that wasn't part of the original code and gets applied to all dialects. When I comment this out, no tests break. - the logic dealing with
operatorsCombine === truecase is not clearly separated from the logic for the base case. - There are lots of conditional statements, and the later ones depend on the earlier ones. So to understand what happens on the last line, one really needs to grasp what happened before on the preceding lines.
There was a problem hiding this comment.
Rewritten as three independent checks, with each hazard explained next to the regex that describes it. No condition depends on the one before it, and operatorsCombine is its own branch.
On item.startsWith('+'): it only ever fired under operatorsCombine, but you had to read down to the last line to see that, which is a fair complaint. It is reachable rather than dead, though: %+ is as valid an operator name as %-, so 5 % +2 densed to 5%+2 re-parses as the operator %+. It was simply untested. It is now -- narrowing the regex to /^[-]/u fails 15 tests.
| private items: LayoutItem[] = []; | ||
|
|
||
| constructor(public indentation: Indentation) {} | ||
| constructor(public indentation: Indentation, private operatorsCombine = false) {} |
There was a problem hiding this comment.
There's now this new flag operatorsCombine on the Layout class.
But there is InlineLayout class which extends from Layout. It seems that the context of InlineLayout that flag is now always false. I have a suspicion that this might cause problems. Maybe you have done it intentionally so. Maybe not. I don't know. But it looks suspicious.
FYI: InlineLayout is used for formatting a sequence of tokens between a pair of parenthesis, trying to fit them on a single line.
There was a problem hiding this comment.
Passed through now, and Layout no longer defaults it, so neither construction site can get false by accident.
It doesn't change any output, though, and I'd rather say that than imply it fixed a bug: formatParenthesis splices the inline layout's items back into the outer layout, which applies the guard again. Setting the flag back to false after this change leaves all 6233 tests green.
There is a real gap next to it that the flag does not reach. addToLength measures the items passed to add(), so it never counts the space the base class inserts. On master, with denseOperators and the default expressionWidth: 50, SELECT * FROM t WHERE (aaa...aaa - -1 - -2) (43 a's, mysql) renders 51 characters inside the parens. This PR makes PostgreSQL reach the same path. Fixing it means measuring what was appended rather than what was passed in, which reflows more than this PR should -- separate PR or an issue, whichever you prefer.
|
|
||
| it('does not space a sign after an operator in dense mode', () => { | ||
| expect(format('SELECT 5 % -2, 5 & -2', { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| 5%-2, | ||
| 5&-2 | ||
| `); | ||
| }); |
There was a problem hiding this comment.
Why this test for MySQL in particular?
I think what you really intended was to write a test for other dialects that don't have the operator-combining problem. But for some reason you just added one test for one specific other dialect.
We should do better than that.
There was a problem hiding this comment.
Agreed, one arbitrary dialect proved nothing. Moved into supportsOperators(), so every dialect that doesn't combine operators now asserts it for each operator it declares: 363 cases across 18 dialects, suite time unchanged.
Control that they bite: setting operatorsCombine: true on MySQL turns 9 of them red.
| // Every character that lets PostgreSQL lex a run as a single operator can swallow a | ||
| // following sign, so each one is checked rather than a sample. The tenth such | ||
| // character, a backtick, is legal in CREATE OPERATOR but the lexer never yields it | ||
| // as an operator, so it is not reachable from here. | ||
| it.each(['~', '!~', '@>', '#', '%', '^', '&', '|', '?'])( | ||
| 'keeps a space between the operator %s and a following sign with denseOperators', | ||
| operator => { | ||
| expect(format(`SELECT a ${operator} -1`, { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| a${operator} -1 | ||
| `); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Like with a previous case of single-test-for-MySQL, here's a similar single test for PostgreSQL. But you enable the operatorsCombine flag also for Redshift. So this thing is not only specific for PostgreSQL.
Interestingly Redshift doesn't quite support such a wide range of operators as PostgreSQL. (Or at least SQL Formatter doesn't support many.)
That brings to my mind that inside isItemSafeToAppend() method we have these hard-coded regexes to match a wide range of characters. Some of these might not be relevant for Redshift. They might not be harmful, but they might not cause conflicts in the same way. Frankly I really don't know. I'd like to hear your thoughts on this.
Alternative proposal: We could leave Redshift out of this pull request, and focus solely on PostgreSQL. You could then come back with another pull request to solve the problem also for Redshift. But up to you.
There was a problem hiding this comment.
Taking your proposal -- Redshift is out, the dialect file is identical to master again, and it gets its own pull request.
To answer the question: 9 of the 12 operators sql-formatter declares for Redshift contain one of ~!@#%^&|`? (^ % @ |/ ||/ & | ~ ||), so for those the hazard is the same as in PostgreSQL; only <<, >> and :: are untouched by the regex. Redshift forked from PostgreSQL 8.0.2, which already had that lexer rule, so the character set isn't wrong for Redshift -- just wider than its tokenizer can produce.
One thing worth flagging: the new cross-dialect test would otherwise assert 1 % -2 -> 1%-2 for Redshift, writing the behaviour I believe is wrong into the suite. So Redshift is marked as sign-absorbing there and asserts nothing until the follow-up.
| it.each(['~', '!~', '@>', '#', '%', '^', '&', '|', '?'])( | ||
| 'keeps a space between the operator %s and a following sign with denseOperators', | ||
| operator => { | ||
| expect(format(`SELECT a ${operator} -1`, { denseOperators: true })).toBe(dedent` | ||
| SELECT | ||
| a${operator} -1 | ||
| `); | ||
| } | ||
| ); | ||
|
|
||
| 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.
Here we're testing the #, %, ^, @> and ? operators twice with two separate tests.
There was a problem hiding this comment.
Removed -- it was a subset of the other test. There is now one parameterized test for the operators that absorb a sign and one for the operators that don't, with each operator appearing once.
…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.
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.
…haracter Renames wouldMergeIntoOperator to isItemSafeToAppend with the boolean inverted, moves the explanation onto the method as a doc comment, and names the matched run precedingOperatorChars. Drops lastItemEndsWith, which the previous commit left unused. Extends the PostgreSQL test to every operator character that can swallow a following sign instead of a sample of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each hazard is now its own condition, independent of the one before it, and the explanation lives with the regex it describes rather than above the method. Layout no longer defaults operatorsCombine to false, so InlineLayout -- the other Layout construction site -- has to state which dialect it is formatting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Redshift has the same operator-name rule, but a narrower set of operators; it gets its own pull request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fe1cb8f to
8766fbf
Compare
Every dialect that doesn't combine operators now asserts that a sign denses onto each operator it declares, instead of a single MySQL example. For PostgreSQL both classes of operator are covered with both signs, which also removes the duplicated cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8766fbf to
84ab9aa
Compare
|
Pushed, rebased onto master: the guard rewrite, Redshift dropped, and the tests.
The rewrite is behaviour-preserving, and I measured that rather than assuming it: formatting the same 11392 queries before and after (77 operator strings x both signs x 4 shapes x |
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).