Skip to content

Commit f246638

Browse files
TimvdLippecushon
TimvdLippe
authored and
cushon
committed
Fix import reorderer to handle single-line-comments in imports
MOE_MIGRATED_REVID=240781176
1 parent 5e114ca commit f246638

File tree

2 files changed

+144
-22
lines changed

2 files changed

+144
-22
lines changed

‎core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,15 +70,20 @@ private class Import implements Comparable<Import> {
7070
/** The name being imported, for example {@code java.util.List}. */
7171
final String imported;
7272

73-
/** The characters after the final {@code ;}, up to and including the line terminator. */
73+
/**
74+
* The {@code //} comment lines after the final {@code ;}, up to and including the line
75+
* terminator of the last one. Note: In case two imports were separated by a space (which is
76+
* disallowed by the style guide), the trailing whitespace of the first import does not include
77+
* a line terminator.
78+
*/
7479
final String trailing;
7580

7681
/** True if this is {@code import static}. */
7782
final boolean isStatic;
7883

7984
Import(String imported, String trailing, boolean isStatic) {
8085
this.imported = imported;
81-
this.trailing = trailing.trim();
86+
this.trailing = trailing;
8287
this.isStatic = isStatic;
8388
}
8489

@@ -91,7 +96,8 @@ public int compareTo(Import that) {
9196
return this.imported.compareTo(that.imported);
9297
}
9398

94-
// This is a complete line to be output for this import, including the line terminator.
99+
// One or multiple lines, the import itself and following comments, including the line
100+
// terminator.
95101
@Override
96102
public String toString() {
97103
StringBuilder sb = new StringBuilder();
@@ -100,10 +106,11 @@ public String toString() {
100106
sb.append("static ");
101107
}
102108
sb.append(imported).append(';');
103-
if (!trailing.isEmpty()) {
104-
sb.append(' ').append(trailing);
109+
if (trailing.trim().isEmpty()) {
110+
sb.append(lineSeparator);
111+
} else {
112+
sb.append(trailing);
105113
}
106-
sb.append(lineSeparator);
107114
return sb.toString();
108115
}
109116
}
@@ -175,7 +182,7 @@ private static class ImportsAndIndex {
175182
* <imports> -> (<end-of-line> | <import>)*
176183
* <import> -> "import" <whitespace> ("static" <whitespace>)?
177184
* <identifier> ("." <identifier>)* ("." "*")? <whitespace>? ";"
178-
* <whitespace>? <line-comment>? <end-of-line>
185+
* <whitespace>? <end-of-line>? (<line-comment> <end-of-line>)*
179186
* }</pre>
180187
*
181188
* @param i the index to start parsing at.
@@ -221,13 +228,19 @@ private ImportsAndIndex scanImports(int i) throws FormatterException {
221228
trailing.append(tokenAt(i));
222229
i++;
223230
}
224-
if (isSlashSlashCommentToken(i)) {
231+
if (isNewlineToken(i)) {
225232
trailing.append(tokenAt(i));
226233
i++;
227234
}
228-
if (isNewlineToken(i)) {
235+
// Gather (if any) all single line comments and accompanied line terminators following this
236+
// import
237+
while (isSlashSlashCommentToken(i)) {
229238
trailing.append(tokenAt(i));
230239
i++;
240+
if (isNewlineToken(i)) {
241+
trailing.append(tokenAt(i));
242+
i++;
243+
}
231244
}
232245
imports.add(new Import(importedName, trailing.toString(), isStatic));
233246
// Remember the position just after the import we just saw, before skipping blank lines.

‎core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java‎

Lines changed: 122 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,128 @@ public static Collection<Object[]> parameters() {
104104
"public class Blim {}",
105105
},
106106
},
107+
{
108+
{
109+
"import java.util.Collection;",
110+
"// BUG: diagnostic contains",
111+
"import java.util.List;",
112+
"",
113+
"class B74235047 {}"
114+
},
115+
{
116+
"import java.util.Collection;",
117+
"// BUG: diagnostic contains",
118+
"import java.util.List;",
119+
"",
120+
"class B74235047 {}"
121+
}
122+
},
123+
{
124+
{
125+
"import java.util.Set;",
126+
"import java.util.Collection;",
127+
"// BUG: diagnostic contains",
128+
"import java.util.List;",
129+
"",
130+
"class B74235047 {}"
131+
},
132+
{
133+
"import java.util.Collection;",
134+
"// BUG: diagnostic contains",
135+
"import java.util.List;",
136+
"import java.util.Set;",
137+
"",
138+
"class B74235047 {}"
139+
}
140+
},
141+
{
142+
{
143+
"import java.util.List;",
144+
"// BUG: diagnostic contains",
145+
"import java.util.Set;",
146+
"import java.util.Collection;",
147+
"",
148+
"class B74235047 {}"
149+
},
150+
{
151+
"import java.util.Collection;",
152+
"import java.util.List;",
153+
"// BUG: diagnostic contains",
154+
"import java.util.Set;",
155+
"",
156+
"class B74235047 {}"
157+
}
158+
},
159+
{
160+
{
161+
"// BEGIN-STRIP",
162+
"import com.google.testing.testsize.MediumTest;",
163+
"import com.google.testing.testsize.MediumTestAttribute;",
164+
"// END-STRIP",
165+
"",
166+
"class B74235047 {}"
167+
},
168+
{
169+
"// BEGIN-STRIP",
170+
"import com.google.testing.testsize.MediumTest;",
171+
"import com.google.testing.testsize.MediumTestAttribute;",
172+
"// END-STRIP",
173+
"",
174+
"class B74235047 {}"
175+
}
176+
},
177+
{
178+
{
179+
"import com.google.testing.testsize.MediumTest; // Keep this import",
180+
"import com.google.testing.testsize.MediumTestAttribute; // Keep this import",
181+
"",
182+
"class B74235047 {}"
183+
},
184+
{
185+
"import com.google.testing.testsize.MediumTest; // Keep this import",
186+
"import com.google.testing.testsize.MediumTestAttribute; // Keep this import",
187+
"",
188+
"class B74235047 {}"
189+
}
190+
},
191+
{
192+
{
193+
"import java.util.Set;",
194+
"import java.util.List;",
195+
"",
196+
"// This comment doesn't get moved because of the blank line.",
197+
"",
198+
"class B74235047 {}"
199+
},
200+
{
201+
"import java.util.List;",
202+
"import java.util.Set;",
203+
"",
204+
"// This comment doesn't get moved because of the blank line.",
205+
"",
206+
"class B74235047 {}"
207+
}
208+
},
209+
{
210+
{
211+
"import b.B;",
212+
"// MOE: end_strip",
213+
"import c.C;",
214+
"// MOE: begin_strip",
215+
"import a.A;",
216+
"",
217+
"class B74235047 {}"
218+
},
219+
{
220+
"import a.A;",
221+
"import b.B;",
222+
"// MOE: end_strip",
223+
"import c.C;",
224+
"// MOE: begin_strip",
225+
"",
226+
"class B74235047 {}"
227+
}
228+
},
107229
{
108230
{
109231
// Check double semicolons
@@ -256,19 +378,6 @@ public static Collection<Object[]> parameters() {
256378
"!!Could not parse imported name, at: ",
257379
}
258380
},
259-
{
260-
{
261-
"package com.google.example;",
262-
"",
263-
"import com.foo.Second;",
264-
"import com.foo.First;",
265-
"// we don't support comments between imports",
266-
"import com.foo.Third;",
267-
},
268-
{
269-
"!!Imports not contiguous (perhaps a comment separates them?)",
270-
}
271-
},
272381
{
273382
{
274383
"import com.foo.Second;",

0 commit comments

Comments
 (0)