From 8f96dc68369fb1b2e911258bf4eb746ecfb8d7bb Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Thu, 9 Jan 2025 16:03:16 -0500 Subject: [PATCH 1/6] follow the API guidelines and only update when formatted file is changed --- .../PalantirJavaFormatFormattingService.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java index e28a716bd..0ac89bc75 100644 --- a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java +++ b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java @@ -89,10 +89,17 @@ public void run() { } try { - String formattedText = applyReplacements( - request.getDocumentText(), - formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request))); - request.onTextReady(formattedText); + List replacements = + formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request)); + + // The Javadoc of onTextReady API says that you should set it to null when the + // document is unchanged. But an even better version is to simply not attempt + // to format a document that is already formatted + if (replacements.isEmpty()) { + return; + } + + request.onTextReady(applyReplacements(request.getDocumentText(), replacements)); } catch (FormatterException e) { request.onError( Notifications.PARSING_ERROR_TITLE, From b2fb695f954c98bedd928530efd6af6d5cc84869 Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Thu, 6 Mar 2025 13:43:45 -0500 Subject: [PATCH 2/6] added a test that exemplifies replacements chaning formatted files --- .../javaformat/java/FormatterTest.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index 1f2da75ac..45cbd9b97 100644 --- a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.base.Joiner; +import com.google.common.collect.Range; import com.google.common.io.CharStreams; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -31,6 +32,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -443,4 +445,29 @@ void canParse_java9_private_interface_methods() { + "}")) .doesNotThrowAnyException(); } + + @Test + void doesntProduceFormattingChangesOnFormattedFiles() throws FormatterException { + Formatter formatter = Formatter.create(); + String simpleClass = "package com.palantir;\n" + + "\n" + + "public final class Formatted {\n" + + " public static int count(int number) {\n" + + " return number;\n" + + " }\n" + + "}"; + // This is kinda overkill but the point of the test is to show that it + // is for sure formatted and still producing a replacement. + String formattedClass = formatter.formatSourceAndFixImports(simpleClass); + String reformattedClass = formatter.formatSourceAndFixImports(formattedClass); + + // The formatSourceAndFixImports code path flows through getFormatReplacements and + // this shows that despite the replacements happening they are superfluous + assertThat(formattedClass).isEqualTo(reformattedClass); + + // Proof that the replacements are produced when fully formatted + assertThat(formatter.getFormatReplacements( + formattedClass, List.of(Range.closedOpen(0, formattedClass.length())))) + .isEmpty(); + } } From 7a180baf8171b0b576b825a6a950446d222c815b Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Tue, 14 Oct 2025 14:30:34 -0400 Subject: [PATCH 3/6] revert back to the naive implementation, note the reason --- .../PalantirJavaFormatFormattingService.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java index 0ac89bc75..407872844 100644 --- a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java +++ b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java @@ -88,18 +88,24 @@ public void run() { return; } + String preFormatText = request.getDocumentText(); + try { - List replacements = - formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request)); - - // The Javadoc of onTextReady API says that you should set it to null when the - // document is unchanged. But an even better version is to simply not attempt - // to format a document that is already formatted - if (replacements.isEmpty()) { - return; + // Note: I attempted to implement this using getFormatReplacements and elide an update when + // there were no replacements. There is a bug in the underlying formatter where it _always_ + // returns a change. Thus we must perform a content aware diff / branching. + String formattedText = applyReplacements( + request.getDocumentText(), + formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request))); + + // The Javadoc of this API says that you should set it to null when the document is unchanged + // We should not be trying to format a document that is already formatted + if (preFormatText.equals(formattedText)) { + request.onTextReady(null); + } else { + request.onTextReady(formattedText); } - request.onTextReady(applyReplacements(request.getDocumentText(), replacements)); } catch (FormatterException e) { request.onError( Notifications.PARSING_ERROR_TITLE, From a1dbad0cbdc34279436d319f4b7430486b9eb24c Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Tue, 14 Oct 2025 14:35:32 -0400 Subject: [PATCH 4/6] cleanups, disable example test --- .../intellij/PalantirJavaFormatFormattingService.java | 3 +-- .../test/java/com/palantir/javaformat/java/FormatterTest.java | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java index b59f4582b..9b848fd9f 100644 --- a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java +++ b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java @@ -36,7 +36,6 @@ import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import org.jetbrains.annotations.NotNull; class PalantirJavaFormatFormattingService extends AsyncDocumentFormattingService { @@ -103,7 +102,7 @@ public void run() { request.getDocumentText().length(), request.getFormattingRanges())); } - + // Note: I attempted to implement this using getFormatReplacements and elide an update when // there were no replacements. There is a bug in the underlying formatter where it _always_ // returns a change. Thus we must perform a content aware diff / branching. diff --git a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index caf5c06d7..04b48d5c4 100644 --- a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -33,6 +33,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.List; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -455,6 +456,7 @@ void canParse_java9_private_interface_methods() { .doesNotThrowAnyException(); } + @Disabled("Disabled as this is proof that PJF always formats the file") @Test void doesntProduceFormattingChangesOnFormattedFiles() throws FormatterException { Formatter formatter = Formatter.create(); From 3269d6952e5fe90d49cda1782cce2797a0325c7e Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Wed, 15 Oct 2025 11:10:20 -0400 Subject: [PATCH 5/6] small refactor feedback --- .../intellij/PalantirJavaFormatFormattingService.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java index 9b848fd9f..2a02a5a8d 100644 --- a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java +++ b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java @@ -99,7 +99,7 @@ public void run() { Optional.ofNullable(request.getIOFile()) .map(file -> file.toPath().toString()) .orElse("null"), - request.getDocumentText().length(), + preFormatText.length(), request.getFormattingRanges())); } @@ -107,8 +107,7 @@ public void run() { // there were no replacements. There is a bug in the underlying formatter where it _always_ // returns a change. Thus we must perform a content aware diff / branching. String formattedText = applyReplacements( - request.getDocumentText(), - formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request))); + preFormatText, formatterService.get().getFormatReplacements(preFormatText, toRanges(request))); // The Javadoc of this API says that you should set it to null when the document is unchanged // We should not be trying to format a document that is already formatted From ef9aefec950429e034205c6c24e46b6eca514bfb Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Wed, 15 Oct 2025 11:10:41 -0400 Subject: [PATCH 6/6] make it clear what action should be taken if the test fails --- .../palantir/javaformat/java/FormatterTest.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index 04b48d5c4..1d7066741 100644 --- a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -33,7 +33,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.List; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -456,9 +455,8 @@ void canParse_java9_private_interface_methods() { .doesNotThrowAnyException(); } - @Disabled("Disabled as this is proof that PJF always formats the file") @Test - void doesntProduceFormattingChangesOnFormattedFiles() throws FormatterException { + void producesFormattingChangesOnAlreadyFormattedFiles() throws FormatterException { Formatter formatter = Formatter.create(); String simpleClass = "package com.palantir;\n" + "\n" @@ -476,9 +474,16 @@ void doesntProduceFormattingChangesOnFormattedFiles() throws FormatterException // this shows that despite the replacements happening they are superfluous assertThat(formattedClass).isEqualTo(reformattedClass); - // Proof that the replacements are produced when fully formatted - assertThat(formatter.getFormatReplacements( + assertWithMessage(""" + If this test is failing and you are reading this message it means that an underlying bug + in the formatting service was fixed. Previously, this formatter always produced a "replacement" + even when the document was fully formatted. You can see evidence of this in + https://github.com/palantir/palantir-java-format/pull/1188. To fix this failing test, consider + relying on the new behavior and instead checking that the 'replacements' is empty instead of + full string comparison. + """) + .that(formatter.getFormatReplacements( formattedClass, List.of(Range.closedOpen(0, formattedClass.length())))) - .isEmpty(); + .isNotEmpty(); } }