-
Notifications
You must be signed in to change notification settings - Fork 457
fix: keep a space between an operator and a following sign with denseOperators #962
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | WS.MANDATORY | |
| export default class Layout { | ||
| private items: LayoutItem[] = []; | ||
|
|
||
| constructor(public indentation: Indentation) {} | ||
| constructor(public indentation: Indentation, private operatorsCombine = false) {} | ||
|
|
||
| /** | ||
| * Appends token strings and whitespace modifications to SQL string. | ||
|
|
@@ -57,10 +57,13 @@ export default class Layout { | |
| this.items.push(WS.SINGLE_INDENT); | ||
| break; | ||
| default: | ||
| // Don't glue a layout item starting with "-" directly onto one ending with | ||
| // "-": that forms "--", which re-parses as a line comment and | ||
| // swallows the rest of the line (e.g. densing "a - -b" into "a--b"). | ||
| if (item.startsWith('-') && this.lastItemEndsWith('-')) { | ||
| // 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) in every dialect. In dialects | ||
| // that lex a run of operator characters as a single operator (PostgreSQL, | ||
| // Redshift), a sign onto an operator containing ~!@#%^&|`? also merges | ||
| // (e.g. densing "5 % -2" into "5%-2", which re-parses as the operator "%-"). | ||
| if (this.wouldMergeIntoOperator(item)) { | ||
| this.items.push(WS.SPACE); | ||
| } | ||
| this.items.push(item); | ||
|
|
@@ -73,6 +76,24 @@ export default class Layout { | |
| return typeof lastItem === 'string' && lastItem.endsWith(suffix); | ||
| } | ||
|
|
||
| private wouldMergeIntoOperator(item: string): boolean { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the name of this method to be kinda awkward. Definitely this code base doesn't contain any other function names starting with I'd suggest inverting the boolean return value of this function and naming it something like |
||
| if (!item.startsWith('-') && !item.startsWith('+')) { | ||
| return false; | ||
| } | ||
| const lastItem = last(this.items); | ||
| if (typeof lastItem !== 'string') { | ||
| return false; | ||
| } | ||
| const run = /[-+*/<>=~!@#%^&|`?]+$/u.exec(lastItem)?.[0]; | ||
| if (!run) { | ||
| return false; | ||
| } | ||
|
Comment on lines
+87
to
+90
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no idea what's going on in here. What does the I guess it's some sort of run of characters. But that doesn't really help me in understanding its purpose. |
||
| if (item.startsWith('-') && run.endsWith('-')) { | ||
| return true; | ||
| } | ||
| return this.operatorsCombine && /[~!@#%^&|`?]/u.test(run); | ||
| } | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| private trimHorizontalWhitespace() { | ||
| while (isHorizontalWhitespace(last(this.items))) { | ||
| this.items.pop(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,6 +234,25 @@ describe('PostgreSqlFormatter', () => { | |
| `); | ||
| }); | ||
|
|
||
| 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 | ||
|
Comment on lines
+237
to
+252
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I counted 17 special characters in the regular expression. In this test we're only checking a few of them. |
||
| `); | ||
| }); | ||
|
|
||
| // Issue #813 | ||
| it('supports OR REPLACE in CREATE FUNCTION', () => { | ||
| expect(format(`CREATE OR REPLACE FUNCTION foo ();`)).toBe(dedent` | ||
|
|
||
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.
This long comment would really be better rewritten as a comment on the
wouldMergeIntoOperator()method, describing what that method does.