Skip to content

Commit b445927

Browse files
abashevabashev
committed
Reflow long strings in formatDiff on the JVM, as with the native binary
The Gradle plugin loads FormatterServiceImpl into its own JVM, and its getFormatReplacements never reflowed strings, while the bootstrapping and native services run Main --output-replacements, which does. formatDiff without the native binary left long strings on one line, and the GitHub Action failed on the result (#219). FormatterServiceImpl.getFormatReplacements now returns what --output-replacements prints. FormatterServiceImplTest compares the two for each line of its sources, and FormatterServicesTest and FormatDiffTest run the in-process formatter next to the bootstrapping and native ones, which they never did.
1 parent 68e5bdf commit b445927

5 files changed

Lines changed: 149 additions & 6 deletions

File tree

‎gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/FormatDiffTest.java‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,7 @@ void parsing_git_diff_output_works() throws IOException {
6969
@MethodSource("getFormatters")
7070
void reformat_a_subpath_of_a_git_directory_for_only_changed_lines(FormatterService formatterService)
7171
throws IOException, InterruptedException {
72-
runCommandInRepo("git", "init");
73-
runCommandInRepo("git", "config", "user.name", "Test User");
74-
runCommandInRepo("git", "config", "user.email", "test-user@palantir.com");
75-
runCommandInRepo("git", "config", "commit.gpgsign", "false");
76-
runCommandInRepo("git", "commit", "--allow-empty", "-m", "Init");
72+
initRepo();
7773

7874
Path subdir = repo.resolve("subdir");
7975
Files.createDirectories(subdir);
@@ -92,6 +88,52 @@ void reformat_a_subpath_of_a_git_directory_for_only_changed_lines(FormatterServi
9288
assertThat(dontTouchMe).hasContent(" class DontTouchMe {}");
9389
}
9490

91+
@ParameterizedTest
92+
@MethodSource("getFormatters")
93+
void reflows_the_long_strings_of_changed_lines_only(FormatterService formatterService)
94+
throws IOException, InterruptedException {
95+
initRepo();
96+
String outside = " String outside = \"Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris"
97+
+ " nisi ut aliquip ex ea commodo consequat sed.\";";
98+
Path longStrings = repo.resolve("LongStrings.java");
99+
Files.write(longStrings, List.of("class LongStrings {", outside, "}"), UTF_8);
100+
runCommandInRepo("git", "add", ".");
101+
runCommandInRepo("git", "commit", "-m", "A long string");
102+
103+
Files.write(
104+
longStrings,
105+
List.of(
106+
"class LongStrings {",
107+
outside,
108+
"",
109+
" String changed = \"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod"
110+
+ " tempor incididunt ut labore et dolore magna.\";",
111+
"}"),
112+
UTF_8);
113+
114+
FormatDiff.formatDiff(repo, formatterService);
115+
116+
assertThat(longStrings)
117+
.hasContent(String.join(
118+
"\n",
119+
"class LongStrings {",
120+
outside,
121+
"",
122+
" String changed =",
123+
" \"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor"
124+
+ " incididunt ut labore et\"",
125+
" + \" dolore magna.\";",
126+
"}"));
127+
}
128+
129+
private void initRepo() throws IOException, InterruptedException {
130+
runCommandInRepo("git", "init");
131+
runCommandInRepo("git", "config", "user.name", "Test User");
132+
runCommandInRepo("git", "config", "user.email", "test-user@palantir.com");
133+
runCommandInRepo("git", "config", "commit.gpgsign", "false");
134+
runCommandInRepo("git", "commit", "--allow-empty", "-m", "Init");
135+
}
136+
95137
private void runCommandInRepo(String... args) throws IOException, InterruptedException {
96138
Process process =
97139
new ProcessBuilder().command(args).directory(repo.toFile()).start();
@@ -107,6 +149,9 @@ private void runCommandInRepo(String... args) throws IOException, InterruptedExc
107149

108150
private static Stream<FormatterService> getFormatters() throws IOException {
109151
return Stream.of(
152+
// In this JVM, as formatDiff runs it without the native binary.
153+
FormatterServiceLoader.load(
154+
getClasspath().stream().map(Path::toFile).toList()),
110155
new BootstrappingFormatterService(
111156
javaBinPath(), Runtime.version().feature(), getClasspath()),
112157
new NativeImageFormatterService(

‎open-java-format-jdk-bootstrap/src/test/java/com/palantir/javaformat/bootstrap/FormatterServicesTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.palantir.javaformat.Utils;
2525
import com.palantir.javaformat.java.FormatterException;
2626
import com.palantir.javaformat.java.FormatterService;
27+
import com.palantir.javaformat.java.FormatterServiceImpl;
2728
import com.palantir.javaformat.java.Replacement;
2829
import java.io.File;
2930
import java.net.URI;
@@ -96,13 +97,47 @@ public void main( String aaaa ) {
9697
.isEmpty();
9798
}
9899

100+
@ParameterizedTest
101+
@MethodSource("getFormattersAndTheInProcessOne")
102+
void reflows_the_long_strings_of_the_ranges_only(FormatterService formatterService) throws FormatterException {
103+
String input = """
104+
class S {
105+
String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed.";
106+
107+
String changed = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna.";
108+
}
109+
""";
110+
String expectedOutput = """
111+
class S {
112+
String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed.";
113+
114+
String changed =
115+
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et"
116+
+ " dolore magna.";
117+
}
118+
""";
119+
int start = input.indexOf(" String changed");
120+
int end = input.indexOf('\n', start);
121+
122+
List<Replacement> replacements =
123+
formatterService.getFormatReplacements(input, List.of(Range.closedOpen(start, end)));
124+
125+
assertThat(Utils.applyReplacements(input, replacements)).isEqualTo(expectedOutput);
126+
}
127+
99128
private static Stream<FormatterService> getFormatters() {
100129
return Stream.of(
101130
new BootstrappingFormatterService(
102131
javaBinPath(), Runtime.version().feature(), getClasspath()),
103132
new NativeImageFormatterService(Path.of(System.getenv("NATIVE_IMAGE_CLASSPATH"))));
104133
}
105134

135+
// With the one that formats in this JVM, as formatDiff does without the native binary. On input that does not parse
136+
// it throws, where the other two return no replacements, so it is not among getFormatters.
137+
private static Stream<FormatterService> getFormattersAndTheInProcessOne() {
138+
return Stream.concat(Stream.of(new FormatterServiceImpl()), getFormatters());
139+
}
140+
106141
private String getTestResourceContent(String resourceName) {
107142
try {
108143
URI resource = getClass().getClassLoader().getResource(resourceName).toURI();

‎open-java-format-spi/src/main/java/com/palantir/javaformat/java/FormatterService.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ public interface FormatterService {
2828
/**
2929
* Emit a list of {@link Replacement}s to convert from input to formatted output.
3030
*
31+
* <p>The ranges are formatted, and the long strings in the code that formatting them rewrites are reflowed; the
32+
* imports, and the strings elsewhere, stay as they are.
33+
*
3134
* @param input the input compilation unit
3235
* @param ranges the character ranges to reformat
3336
* @return a list of {@link Replacement}s, sorted from low index to high index, without overlaps

‎open-java-format/src/main/java/com/palantir/javaformat/java/FormatterServiceImpl.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.auto.service.AutoService;
1919
import com.google.common.collect.ImmutableList;
2020
import com.google.common.collect.Range;
21+
import com.google.common.collect.TreeRangeSet;
2122
import com.palantir.javaformat.java.JavaFormatterOptions.Style;
2223
import java.util.Collection;
2324

@@ -36,7 +37,8 @@ public FormatterServiceImpl() {
3637
@SuppressForbidden(reason = "FormatterService, kept in the SPI for compatibility, returns ImmutableList")
3738
public ImmutableList<Replacement> getFormatReplacements(String text, Collection<Range<Integer>> toRanges)
3839
throws FormatterException {
39-
return ImmutableList.copyOf(formatter.getFormatReplacements(text, toRanges));
40+
// What the command line prints for --output-replacements, which the bootstrapping and native services run.
41+
return ImmutableList.copyOf(formatter.getFormatAndReflowReplacements(text, TreeRangeSet.create(toRanges)));
4042
}
4143

4244
@Override

‎open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,16 @@
1818
import static org.assertj.core.api.Assertions.assertThat;
1919
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2020

21+
import com.fasterxml.jackson.core.type.TypeReference;
22+
import com.fasterxml.jackson.databind.ObjectMapper;
23+
import com.fasterxml.jackson.databind.json.JsonMapper;
24+
import com.fasterxml.jackson.datatype.guava.GuavaModule;
25+
import com.google.common.collect.Range;
26+
import com.palantir.javaformat.Utils;
2127
import java.io.ByteArrayInputStream;
2228
import java.io.PrintWriter;
2329
import java.io.StringWriter;
30+
import java.util.List;
2431
import java.util.stream.Stream;
2532
import org.junit.jupiter.api.Test;
2633
import org.junit.jupiter.params.ParameterizedTest;
@@ -34,6 +41,8 @@
3441
*/
3542
final class FormatterServiceImplTest {
3643
private static final FormatterService SERVICE = new FormatterServiceImpl();
44+
private static final ObjectMapper MAPPER =
45+
JsonMapper.builder().addModule(new GuavaModule()).build();
3746

3847
// Two long strings, as in a file where only the line of the second one has changed.
3948
private static final String LONG_STRINGS = """
@@ -81,6 +90,55 @@ void formatsAFileAsTheCommandLineDoes(String name, String source) throws Excepti
8190
assertThat(SERVICE.formatSourceReflowStringsAndFixImports(source)).isEqualTo(commandLine(source, "-"));
8291
}
8392

93+
@ParameterizedTest(name = "{0}")
94+
@MethodSource("sources")
95+
void replacesEachLineAsTheCommandLineDoes(String name, String source) throws Exception {
96+
int start = 0;
97+
for (String line : source.split("\n", -1)) {
98+
int end = start + line.length();
99+
if (!line.isBlank()) {
100+
List<Replacement> printed = MAPPER.readValue(
101+
commandLine(
102+
source, "--character-ranges", start + ":" + (end - 1), "--output-replacements", "-"),
103+
new TypeReference<>() {});
104+
assertThat(SERVICE.getFormatReplacements(source, List.of(Range.closedOpen(start, end))))
105+
.as(line)
106+
.isEqualTo(printed);
107+
}
108+
start = end + 1;
109+
}
110+
}
111+
112+
@Test
113+
void reflowsTheLongStringOfAChangedLineOnly() throws Exception {
114+
String changed = LONG_STRINGS
115+
.lines()
116+
.filter(line -> line.contains("changed"))
117+
.findFirst()
118+
.orElseThrow();
119+
int start = LONG_STRINGS.indexOf(changed);
120+
int end = start + changed.length();
121+
122+
List<Replacement> replacements =
123+
SERVICE.getFormatReplacements(LONG_STRINGS, List.of(Range.closedOpen(start, end)));
124+
// The command line used to reflow every long string in the file once one in the ranges needed it.
125+
List<Replacement> printed = MAPPER.readValue(
126+
commandLine(LONG_STRINGS, "--character-ranges", start + ":" + (end - 1), "--output-replacements", "-"),
127+
new TypeReference<>() {});
128+
129+
assertThat(Utils.applyReplacements(LONG_STRINGS, printed))
130+
.isEqualTo(Utils.applyReplacements(LONG_STRINGS, replacements));
131+
assertThat(Utils.applyReplacements(LONG_STRINGS, replacements)).isEqualTo("""
132+
class S {
133+
String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed.";
134+
135+
String changed =
136+
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et"
137+
+ " dolore magna.";
138+
}
139+
""");
140+
}
141+
84142
@Test
85143
void failsOnAnExtraneousSemicolonAsTheCommandLineDoes() throws Exception {
86144
// Fixing the imports first used to drop the semicolon, which javac 21 rejects between imports, and format

0 commit comments

Comments
 (0)