Skip to content

Commit cf24534

Browse files
nenenene
authored
Merge PR #376: Fix semicolon-on-separate-line bug
2 parents 6611834 + 8fbadfa commit cf24534

File tree

9 files changed

+196
-21
lines changed

9 files changed

+196
-21
lines changed

‎src/formatter/ExpressionFormatter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ export default class ExpressionFormatter {
231231

232232
/** Formats a line comment onto query */
233233
private formatLineComment(token: Token) {
234-
this.layout.add(this.show(token), WS.NEWLINE, WS.INDENT);
234+
this.layout.add(this.show(token), WS.MANDATORY_NEWLINE, WS.INDENT);
235235
}
236236

237237
/** Formats a block comment onto query */

‎src/formatter/Formatter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export default class Formatter {
7272
} else if (this.cfg.newlineBeforeSemicolon) {
7373
layout.add(WS.NEWLINE, ';');
7474
} else {
75-
layout.add(WS.NO_SPACE, ';');
75+
layout.add(WS.NO_NEWLINE, ';');
7676
}
7777
return layout.toString();
7878
}

‎src/formatter/Layout.ts

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ import Indentation from './Indentation';
66
export enum WS {
77
SPACE, // Adds single space
88
NO_SPACE, // Removes preceding horizontal whitespace (if any)
9+
NO_NEWLINE, // Removes all preceding whitespace (whether horizontal or vertical)
910
NEWLINE, // Adds single newline (and removes any preceding whitespace)
11+
MANDATORY_NEWLINE, // Adds single newline that can't be removed by NO_NEWLINE
1012
INDENT, // Adds indentation (as much as needed for current indentation level)
1113
SINGLE_INDENT, // Adds whitespace for single indentation step
1214
}
1315

14-
export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | string;
16+
export type LayoutItem = WS.SPACE | WS.SINGLE_INDENT | WS.NEWLINE | WS.MANDATORY_NEWLINE | string;
1517

1618
/**
1719
* API for constructing SQL string (especially the whitespace part).
@@ -37,9 +39,16 @@ export default class Layout {
3739
case WS.NO_SPACE:
3840
this.trimHorizontalWhitespace();
3941
break;
42+
case WS.NO_NEWLINE:
43+
this.trimWhitespace();
44+
break;
4045
case WS.NEWLINE:
4146
this.trimHorizontalWhitespace();
42-
this.addNewline();
47+
this.addNewline(WS.NEWLINE);
48+
break;
49+
case WS.MANDATORY_NEWLINE:
50+
this.trimHorizontalWhitespace();
51+
this.addNewline(WS.MANDATORY_NEWLINE);
4352
break;
4453
case WS.INDENT:
4554
this.addIndentation();
@@ -59,9 +68,26 @@ export default class Layout {
5968
}
6069
}
6170

62-
private addNewline() {
63-
if (this.items.length > 0 && last(this.items) !== WS.NEWLINE) {
64-
this.items.push(WS.NEWLINE);
71+
private trimWhitespace() {
72+
while (isRemovableWhitespace(last(this.items))) {
73+
this.items.pop();
74+
}
75+
}
76+
77+
private addNewline(newline: WS.NEWLINE | WS.MANDATORY_NEWLINE) {
78+
if (this.items.length > 0) {
79+
switch (last(this.items)) {
80+
case WS.NEWLINE:
81+
this.items.pop();
82+
this.items.push(newline);
83+
break;
84+
case WS.MANDATORY_NEWLINE:
85+
// keep as is
86+
break;
87+
default:
88+
this.items.push(newline);
89+
break;
90+
}
6591
}
6692
}
6793

@@ -83,6 +109,7 @@ export default class Layout {
83109
case WS.SPACE:
84110
return ' ';
85111
case WS.NEWLINE:
112+
case WS.MANDATORY_NEWLINE:
86113
return '\n';
87114
case WS.SINGLE_INDENT:
88115
return this.indentation.getSingleIndent();
@@ -94,3 +121,6 @@ export default class Layout {
94121

95122
const isHorizontalWhitespace = (item: WS | string | undefined) =>
96123
item === WS.SPACE || item === WS.SINGLE_INDENT;
124+
125+
const isRemovableWhitespace = (item: WS | string | undefined) =>
126+
item === WS.SPACE || item === WS.SINGLE_INDENT || item === WS.NEWLINE;

‎test/behavesLikeMariaDbFormatter.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ export default function behavesLikeMariaDbFormatter(format: FormatFn) {
127127
expect(
128128
format(
129129
`ALTER TABLE t ALTER COLUMN foo SET DEFAULT 10;
130-
ALTER TABLE t ALTER COLUMN foo DROP DEFAULT`
130+
ALTER TABLE t ALTER COLUMN foo DROP DEFAULT;`
131131
)
132132
).toBe(dedent`
133133
ALTER TABLE
@@ -141,7 +141,7 @@ export default function behavesLikeMariaDbFormatter(format: FormatFn) {
141141
t
142142
ALTER COLUMN
143143
foo
144-
DROP DEFAULT
144+
DROP DEFAULT;
145145
`);
146146
});
147147
}

‎test/db2.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ describe('Db2Formatter', () => {
115115
expect(
116116
format(
117117
`ALTER TABLE t ALTER COLUMN foo SET DATA TYPE VARCHAR;
118-
ALTER TABLE t ALTER COLUMN foo SET NOT NULL`
118+
ALTER TABLE t ALTER COLUMN foo SET NOT NULL;`
119119
)
120120
).toBe(dedent`
121121
ALTER TABLE
@@ -129,7 +129,7 @@ describe('Db2Formatter', () => {
129129
t
130130
ALTER COLUMN
131131
foo
132-
SET NOT NULL
132+
SET NOT NULL;
133133
`);
134134
});
135135
});

‎test/options/newlineBeforeSemicolon.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,25 @@ export default function supportsNewlineBeforeSemicolon(format: FormatFn) {
2121
`);
2222
});
2323

24+
// A regression test for semicolon placement in single-line clauses like:
25+
//
26+
// ALTER TABLE
27+
// my_table
28+
// ALTER COLUMN
29+
// foo
30+
// DROP DEFAULT; <-- here
31+
//
32+
// Unfortunately there's really no such single-line clause that exists in all dialects,
33+
// so our test resorts to using somewhat invalid SQL.
34+
it('places semicolon on the same line as a single-line clause', () => {
35+
const result = format(`SELECT a FROM;`);
36+
expect(result).toBe(dedent`
37+
SELECT
38+
a
39+
FROM;
40+
`);
41+
});
42+
2443
it('supports semicolon on separate line', () => {
2544
const result = format(`SELECT a FROM b;`, { newlineBeforeSemicolon: true });
2645
expect(result).toBe(dedent`

‎test/postgresql.test.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ describe('PostgreSqlFormatter', () => {
132132
ALTER TABLE t ALTER COLUMN foo SET DEFAULT 5;
133133
ALTER TABLE t ALTER COLUMN foo DROP DEFAULT;
134134
ALTER TABLE t ALTER COLUMN foo SET NOT NULL;
135-
ALTER TABLE t ALTER COLUMN foo DROP NOT NULL`
135+
ALTER TABLE t ALTER COLUMN foo DROP NOT NULL;`
136136
)
137137
).toBe(dedent`
138138
ALTER TABLE
@@ -153,21 +153,19 @@ describe('PostgreSqlFormatter', () => {
153153
t
154154
ALTER COLUMN
155155
foo
156-
DROP DEFAULT
157-
;
156+
DROP DEFAULT;
158157
159158
ALTER TABLE
160159
t
161160
ALTER COLUMN
162161
foo
163-
SET NOT NULL
164-
;
162+
SET NOT NULL;
165163
166164
ALTER TABLE
167165
t
168166
ALTER COLUMN
169167
foo
170-
DROP NOT NULL
168+
DROP NOT NULL;
171169
`);
172170
});
173171
});

‎test/sql.test.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,13 @@ describe('SqlFormatter', () => {
9191
t
9292
ALTER COLUMN
9393
foo
94-
DROP DEFAULT
95-
;
94+
DROP DEFAULT;
9695
9796
ALTER TABLE
9897
t
9998
ALTER COLUMN
10099
foo
101-
DROP SCOPE CASCADE
102-
;
100+
DROP SCOPE CASCADE;
103101
104102
ALTER TABLE
105103
t

‎test/unit/Layout.test.ts

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import Indentation from 'src/formatter/Indentation';
2+
import Layout, { WS } from 'src/formatter/Layout';
3+
4+
describe('Layout', () => {
5+
function testLayout(...items: (WS | string)[]): string {
6+
// an Indentation object with two steps of indentation
7+
const indentation = new Indentation('-->');
8+
indentation.increaseTopLevel();
9+
indentation.increaseTopLevel();
10+
11+
const layout = new Layout(indentation);
12+
layout.add(...items);
13+
return layout.toString();
14+
}
15+
16+
it('simply concatenates plain strings', () => {
17+
expect(testLayout('hello', 'world')).toBe('helloworld');
18+
});
19+
20+
describe('WS.SPACE', () => {
21+
it('inserts single space', () => {
22+
expect(testLayout('hello', WS.SPACE, 'world')).toBe('hello world');
23+
});
24+
});
25+
26+
describe('WS.SINGLE_INDENT', () => {
27+
it('inserts single indentation step', () => {
28+
expect(testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, 'world')).toBe('hello\n-->world');
29+
});
30+
31+
it('inserts two indentation steps when used twice', () => {
32+
expect(testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, WS.SINGLE_INDENT, 'world')).toBe(
33+
'hello\n-->-->world'
34+
);
35+
});
36+
});
37+
38+
describe('WS.INDENT', () => {
39+
it('inserts current amount of indentation', () => {
40+
expect(testLayout('hello', WS.NEWLINE, WS.INDENT, 'world')).toBe('hello\n-->-->world');
41+
});
42+
43+
it('inserts double the current indentation when used twice', () => {
44+
expect(testLayout('hello', WS.NEWLINE, WS.INDENT, WS.INDENT, 'world')).toBe(
45+
'hello\n-->-->-->-->world'
46+
);
47+
});
48+
});
49+
50+
describe('WS.NO_SPACE', () => {
51+
it('does nothing when no preceding whitespace', () => {
52+
expect(testLayout('hello', WS.NO_SPACE, 'world')).toBe('helloworld');
53+
});
54+
55+
it('removes all preceding spaces', () => {
56+
expect(testLayout('hello', WS.SPACE, WS.SPACE, WS.NO_SPACE, 'world')).toBe('helloworld');
57+
});
58+
59+
it('removes all preceding indentation', () => {
60+
expect(
61+
testLayout('hello', WS.NEWLINE, WS.SINGLE_INDENT, WS.SINGLE_INDENT, WS.NO_SPACE, 'world')
62+
).toBe('hello\nworld');
63+
expect(testLayout('hello', WS.NEWLINE, WS.INDENT, WS.NO_SPACE, 'world')).toBe('hello\nworld');
64+
});
65+
66+
it('does not remove preceding newline', () => {
67+
expect(testLayout('hello', WS.NEWLINE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
68+
expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
69+
});
70+
71+
it('removes preceding spaces up to first newline', () => {
72+
expect(testLayout('hello', WS.NEWLINE, WS.SPACE, WS.NO_SPACE, 'world')).toBe('hello\nworld');
73+
expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.SPACE, WS.NO_SPACE, 'world')).toBe(
74+
'hello\nworld'
75+
);
76+
});
77+
});
78+
79+
describe('WS.NEWLINE', () => {
80+
it('inserts single newline', () => {
81+
expect(testLayout('hello', WS.NEWLINE, 'world')).toBe('hello\nworld');
82+
});
83+
84+
it('inserts single newline, even when used twice', () => {
85+
expect(testLayout('hello', WS.NEWLINE, WS.NEWLINE, 'world')).toBe('hello\nworld');
86+
});
87+
88+
it('trims preceding horizontal whitespace', () => {
89+
expect(testLayout('hello', WS.SPACE, WS.NEWLINE, 'world')).toBe('hello\nworld');
90+
expect(testLayout('hello', WS.INDENT, WS.NEWLINE, 'world')).toBe('hello\nworld');
91+
expect(testLayout('hello', WS.SINGLE_INDENT, WS.NEWLINE, 'world')).toBe('hello\nworld');
92+
});
93+
});
94+
95+
describe('WS.MANDATORY_NEWLINE', () => {
96+
it('inserts single newline', () => {
97+
expect(testLayout('hello', WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
98+
});
99+
100+
it('inserts single newline, even when used twice', () => {
101+
expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe(
102+
'hello\nworld'
103+
);
104+
});
105+
106+
it('inserts single newline, even when used after plain WS.NEWLINE', () => {
107+
expect(testLayout('hello', WS.NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
108+
});
109+
110+
it('inserts single newline, even when used before plain WS.NEWLINE', () => {
111+
expect(testLayout('hello', WS.NEWLINE, WS.MANDATORY_NEWLINE, 'world')).toBe('hello\nworld');
112+
});
113+
});
114+
115+
describe('WS.NO_NEWLINE', () => {
116+
it('removes all preceding spaces', () => {
117+
expect(testLayout('hello', WS.SPACE, WS.NO_NEWLINE, 'world')).toBe('helloworld');
118+
});
119+
120+
it('removes all preceding newlines', () => {
121+
expect(testLayout('hello', WS.NEWLINE, WS.NO_NEWLINE, 'world')).toBe('helloworld');
122+
});
123+
124+
it('does not remove preceding mandatory newlines', () => {
125+
expect(testLayout('hello', WS.MANDATORY_NEWLINE, WS.NO_NEWLINE, 'world')).toBe(
126+
'hello\nworld'
127+
);
128+
});
129+
});
130+
});

0 commit comments

Comments
 (0)