feat: Implement conversion of diff content to ReviewDog format #2478

YongGoose · 2025-05-09T13:35:27Z

Related issue: #655

nedtwigg

Starting a draft PR is a great approach!

The Gradle tests are quite slow. I would create a ReviewDogGenerator in the lib-extra project, running tests there is much faster than in the Gradle project. Once we can generate a ReviewDog file, then we can wire it up with Gradle.

As for the wiring, this is how it works now

graph TD
    spotlessJava --> spotlessJavaCheck
    spotlessJava --> spotlessJavaApply
    spotlessJavaCheck --> spotlessCheck
    spotlessJavaApply --> spotlessApply

    spotlessKotlin --> spotlessKotlinCheck
    spotlessKotlin --> spotlessKotlinApply
    spotlessKotlinCheck --> spotlessCheck
    spotlessKotlinApply --> spotlessApply

All of the work happens in spotlessJava, and it happens in a way which supports caching and up-to-dateness, etc

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java

Lines 77 to 97 in 6f1919f

    
           @TaskAction 
        
           public void performAction(InputChanges inputs) throws Exception { 
        
           	IdeHook.State ideHook = getIdeHookState().getOrNull(); 
        
           	if (ideHook != null && ideHook.path != null) { 
        
           		IdeHook.performHook(this, ideHook); 
        
           		return; 
        
           	} 
        
           	SpotlessTaskService taskService = getTaskService().get(); 
        
           	taskService.registerSourceAlreadyRan(this); 
        
           	if (target == null) { 
        
           		throw new GradleException("You must specify 'Iterable<File> target'"); 
        
           	} 
        
           	if (!inputs.isIncremental()) { 
        
           		getLogger().info("Not incremental: removing prior outputs"); 
        
           		getFs().delete(d -> d.delete(cleanDirectory)); 
        
           		getFs().delete(d -> d.delete(lintsDirectory)); 
        
           		Files.createDirectories(cleanDirectory.toPath()); 
        
           		Files.createDirectories(lintsDirectory.toPath()); 
        
           	}

Then spotlessJavaCheck and spotlessJavaApply just look at those folders and copy them around

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java

Lines 59 to 68 in 6f1919f

    
           private void performAction(boolean isTest) throws IOException { 
        
           	ConfigurableFileTree cleanFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessCleanDirectory().get()); 
        
           	ConfigurableFileTree lintsFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessLintsDirectory().get()); 
        
           	if (cleanFiles.isEmpty() && lintsFiles.isEmpty()) { 
        
           		getState().setDidWork(sourceDidWork()); 
        
           	} else if (!isTest && applyHasRun()) { 
        
           		// if our matching apply has already run, then we don't need to do anything 
        
           		getState().setDidWork(false); 
        
           	} else { 
        
           		List<File> unformattedFiles = getUncleanFiles(cleanFiles);

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessApply.java

Lines 33 to 39 in 6f1919f

    
           public void performAction() { 
        
           	getTaskService().get().registerApplyAlreadyRan(this); 
        
           	ConfigurableFileTree cleanFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessCleanDirectory().get()); 
        
           	ConfigurableFileTree lintsFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessLintsDirectory().get()); 
        
           	if (cleanFiles.isEmpty() && lintsFiles.isEmpty()) { 
        
           		getState().setDidWork(sourceDidWork()); 
        
           	} else {

I would expect spotlessReviewDog to work in that same structure. But probably we should get the guts working in lib-extra, and also write some docs for plugin-gradle, before we spend too much time implementing the wiring.

nedtwigg · 2025-05-09T17:57:07Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java

+			task.setGroup(TASK_GROUP);
+			task.mustRunAfter(BasePlugin.CLEAN_TASK_NAME);
+		});
+		rootCheckTask.configure(task -> task.dependsOn(diffTask));


I don't think everybody needs the diff task.

plugin-gradle/CHANGES.md

YongGoose · 2025-05-11T06:06:05Z

@nedtwigg

I’m planning to implement this feature in three steps:

Create the ReviewDogGenerator class
Add a task in the Gradle-plugin that uses the information stored by ReviewDogGenerator
Write test codes

This dc52b26 includes only the implementation of step 1.

Also, I have a question!

After ReviewDogGenerator generates a diff for ReviewDog, I’d like to store that information in a separate directory (like SpotlessTaskImpl does).
However, it seems that classes from gradle.api can’t be used within the lib-extra directory.

Would you be able to offer some guidance on this?

nedtwigg · 2025-05-12T20:00:38Z

Here is my suggestion:

class ReviewDog {
  public static String rdjsonlDiff(String path, String actualContent, String formattedContent)
  public static String rdjsonlLints(String path, String formattedContent, List<FormatterStep> steps, List<List<Lint>> lintsPerStep)
}

You can test this easily in lib-extra

class ReviewDogTest {
  @Test
  public void diffSingleLine() {
    expectSelfie(ReviewDog.rdjsonlDiff("test.txt", "dirty", "clean")).toBe_TODO()
  }
  ... etc
}

So you can build and test the ReviewDog features in isolation, no gradle API required. Once these are working, you can pipe them together in a Gradle task.

YongGoose · 2025-05-16T11:38:22Z

Here is my suggestion:

class ReviewDog {
  public static String rdjsonlDiff(String path, String actualContent, String formattedContent)
  public static String rdjsonlLints(String path, String formattedContent, List<FormatterStep> steps, List<List<Lint>> lintsPerStep)
}

Sorry for the late reply.

Would you mind explaining the purpose of the ReviewDog class?
I’m still having a bit of trouble understanding it.
To be more specific, I’m curious where actualContext and formattedContent are coming from.

Would it be correct to understand this test as one meant to verify the functionality in isolation?

nedtwigg · 2025-05-21T03:52:38Z

I’m curious where actualContext and formattedContent are coming from

actualContent is the file as it exists
formattedContent is the formatter version of the file

Depending on the plugin integration, they will come from different places. But I promise they are available! If you build it this way, the ReviewDog stuff is decoupled from Formatter etc. You don't have to make fake and irrelevant Formatter instances to test it.

YongGoose · 2025-05-21T04:18:52Z

I’m curious where actualContext and formattedContent are coming from

actualContent is the file as it exists

formattedContent is the formatter version of the file

Depending on the plugin integration, they will come from different places. But I promise they are available! If you build it this way, the ReviewDog stuff is decoupled from Formatter etc. You don't have to make fake and irrelevant Formatter instances to test it.

Based on the comments, I removed the internal formatter and refactored it into a utility class.
PTAL ⚡️

Since I had worked on it previously, I was able to make the changes quickly. 😁

YongGoose · 2025-05-21T04:44:12Z

@nedtwigg

Feel free to update the code if you think any changes are necessary. 🚀

nedtwigg

Great progress! Next step is probably the docs. Try updating the plugin-gradle/README.md with some kind of flag so that failed spotlessCheck will generate the reviewDog files in the appropriate place. Link to setup instructions for reviewDog, etc.

nedtwigg · 2025-05-21T05:13:55Z

lib-extra/src/test/java/com/diffplug/spotless/extra/middleware/ReviewDogGeneratorTest.java

+		assertNotNull(result);
+		assertTrue(result.contains("\"path\":\"src/main.java\""));
+		assertTrue(result.contains("-Dirty line"));
+		assertTrue(result.contains("+Clean line"));


Instead of this, just do Selfie.expectSelfie(result).toBe_TODO(). When you run it, it will replace the TODO with the actual content. Makes the assertion easier to read, write, and maintain.

What a awesome feature!!!
Done in 5724b73 , 745810b !

- use Selfie.expectSelfie(result).toBe_TODO()

- use Selfie.expectSelfie(result).toBe_TODO() Signed-off-by: yongjunhong <yongjunh@apache.org>

Signed-off-by: yongjunhong <yongjunh@apache.org>

YongGoose · 2025-05-21T06:10:26Z

@nedtwigg

I've added an initial version of the README. e7015df
I think the gradle code will need to be updated after we complete the implementation.

Please take a look :)

nedtwigg

Good! If you're okay with the docs as specified, the next step is

add reviewDog property to SpotlessExtension
pipe that property to spotlessCheck

you already have the data you need for ReviewDogGenerator

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java

Lines 68 to 85 in b0eec1f

    
           List<File> unformattedFiles = getUncleanFiles(cleanFiles); 
        
           if (!unformattedFiles.isEmpty()) { 
        
           	// if any files are unformatted, we show those 
        
           	throw new GradleException(DiffMessageFormatter.builder() 
        
           			.runToFix(getRunToFixMessage().get()) 
        
           			.formatterFolder( 
        
           					getProjectDir().get().getAsFile().toPath(), 
        
           					getSpotlessCleanDirectory().get().toPath(), 
        
           					getEncoding().get()) 
        
           			.problemFiles(unformattedFiles) 
        
           			.getMessage()); 
        
           } else { 
        
           	// We only show lints if there are no unformatted files. 
        
           	// This is because lint line numbers are relative to the 
        
           	// formatted content, and formatting often fixes lints. 
        
           	boolean detailed = false; 
        
           	throw new GradleException(super.allLintsErrorMsgDetailed(lintsFiles, detailed)); 
        
           }

nedtwigg · 2025-05-21T06:20:14Z

plugin-gradle/README.md

+### Automatic report generation on check failure
+
+To generate reports when `spotlessCheck` fails:
+
+```gradle
+tasks.named('spotlessCheck').configure {
+  ignoreFailures = true
+  finalizedBy tasks.register('generateReviewdogReport') {
+    doLast {
+      if (spotlessCheck.outcome.failure) {
+        logger.lifecycle("Spotless check failed - Reviewdog report generated")
+      }
+    }
+  }
+}
+```


I don't think we should have a separate generateReviewdogReport. If reviewdogOutput is set, then we should generate that output in spotlessCheck

Done in a94b357 !

I’ll revise it again after all implementations are complete.

plugin-gradle/README.md

Signed-off-by: yongjunhong <yongjunh@apache.org>

YongGoose

@nedtwigg

I’ve left comments on the parts where I need help or think we should discuss further.
Please take a look :)

YongGoose · 2025-05-21T14:00:16Z

lib-extra/src/main/java/com/diffplug/spotless/extra/middleware/ReviewDogGenerator.java

+	 * @param lintsPerStep     The list of lints produced by each step
+	 * @return A string in rdjsonl format representing the lints
+	 */
+	public static String rdjsonlLints(String path, List<FormatterStep> steps, List<List<Lint>> lintsPerStep) {


Should formattedContent also be output in the ReviewDog format in rdjsonlLints?

YongGoose · 2025-05-21T14:01:59Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java

Reduced use of deps by returning early.
Did this for readability, but I can revert if you think the previous version was better.
Let me know what you think!

YongGoose · 2025-05-21T14:03:25Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java

+		if (getReviewDog().get()) {
+			for (File file : lintsFiles.getFiles()) {
+				String path = file.getPath();
+				// TODO : send or save the output of ReviewDogGenerator.rdjsonlLints
+				ReviewDogGenerator.rdjsonlLints(path, null, null);


I’m not sure how to get and pass the List<FormatterStep> steps and List<List<Lint>> lintsPerStep objects here.

Hm, this is a bit tricky. I would do it like so

ReviewDogGenerator currently takes List<FormatterStep> steps, but it could also take List<String> stepNames

Create the stepNames method, and make it the "real" one. The List<FormatterStep> one should just create List<String> and delegate

this is because Gradle has some hiccups that make it a lot easier to have a List<String> as an input

Add a property to SpotlessCheck and set it up here

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java

Lines 85 to 90 in b0eec1f

TaskProvider<SpotlessCheck> checkTask = tasks.register(taskName + CHECK, SpotlessCheck.class, task -> {

SpotlessTaskImpl source = spotlessTask.get();

task.setGroup(TASK_GROUP);

task.init(source);

task.setEnabled(ideHook.path == null);

task.dependsOn(source);

Done in a7d515c !

PTAL ⚡️

nedtwigg

Great progress!

nedtwigg · 2025-05-21T15:46:14Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java

+	public void setReviewDog(boolean reviewDog) {
+		this.reviewDog = reviewDog;
+	}


Per the docs:

reviewdogOutput file("${buildDir}/spotless-reviewdog.json")

The boolean you used here is probably better. Consider a multi-project build and each project has both a java and a kotlin reviewdog thing. Probably want something like this, which is easy to do if it's just a boolean flag. Specifying an output file for every task would be very tedious.

app/build/spotless-reviewdog/java/spotless-reviewdog.json app/build/spotless-reviewdog/kotlin/spotless-reviewdog.json lib/build/spotless-reviewdog/java/spotless-reviewdog.json lib/build/spotless-reviewdog/kotlin/spotless-reviewdog.json

And for a successful spotlessCheck, are these files empty? Or do they not exist at all?

Done in 5ae21df !

PTAL ⚡️

nedtwigg · 2025-05-21T16:04:01Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java

+		if (getReviewDog().get()) {
+			for (File file : lintsFiles.getFiles()) {
+				String path = file.getPath();
+				// TODO : send or save the output of ReviewDogGenerator.rdjsonlLints
+				ReviewDogGenerator.rdjsonlLints(path, null, null);


Hm, this is a bit tricky. I would do it like so

ReviewDogGenerator currently takes List<FormatterStep> steps, but it could also take List<String> stepNames

Create the stepNames method, and make it the "real" one. The List<FormatterStep> one should just create List<String> and delegate

this is because Gradle has some hiccups that make it a lot easier to have a List<String> as an input

Add a property to SpotlessCheck and set it up here

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java

Lines 85 to 90 in b0eec1f

TaskProvider<SpotlessCheck> checkTask = tasks.register(taskName + CHECK, SpotlessCheck.class, task -> {

SpotlessTaskImpl source = spotlessTask.get();

task.setGroup(TASK_GROUP);

task.init(source);

task.setEnabled(ideHook.path == null);

task.dependsOn(source);

Signed-off-by: yongjunhong <yongjunh@apache.org>

… objects Signed-off-by: yongjunhong <yongjunh@apache.org>

YongGoose · 2025-06-01T02:28:17Z

@nedtwigg

Hello! Hope you’re having a great day 🙂
When you have some time, would you mind taking a look and sharing your feedback?

YongGoose added 3 commits May 9, 2025 22:24

feat: Implement conversion of diff content to ReviewDog format

e44435b

Related issue: diffplug#655

Add changes

31ab0b9

Add license

10c23f4

YongGoose mentioned this pull request May 9, 2025

spotlessCheck could add comments and suggested fix to GitHub PR #655

Open

nedtwigg requested changes May 9, 2025

View reviewed changes

YongGoose added 3 commits May 11, 2025 12:53

Revert changes

6d8b7e8

Apply comment

782bd94

Add ReviewDogGenerator

dc52b26

YongGoose requested a review from nedtwigg May 11, 2025 12:19

Apply spotless

25a4c27

Apply comment

17ef58e

Update license header

b63253d

YongGoose added 2 commits May 21, 2025 13:45

Remove unused code

dfd422f

Refactor some codes

fb5aa2d

nedtwigg reviewed May 21, 2025

View reviewed changes

YongGoose added 4 commits May 21, 2025 14:21

Refactor test codes

745810b

- use Selfie.expectSelfie(result).toBe_TODO()

Refactor test codes

5724b73

- use Selfie.expectSelfie(result).toBe_TODO() Signed-off-by: yongjunhong <yongjunh@apache.org>

Apply spotless

62eb863

Signed-off-by: yongjunhong <yongjunh@apache.org>

Update README.md

e7015df

Signed-off-by: yongjunhong <yongjunh@apache.org>

nedtwigg reviewed May 21, 2025

View reviewed changes

YongGoose added 2 commits May 21, 2025 21:34

Update README.md

a94b357

Signed-off-by: yongjunhong <yongjunh@apache.org>

Add reviewDog property and pipe it to spotlessCheck

6688b33

Signed-off-by: yongjunhong <yongjunh@apache.org>

YongGoose commented May 21, 2025

View reviewed changes

YongGoose requested a review from nedtwigg May 21, 2025 14:04

nedtwigg reviewed May 21, 2025

View reviewed changes

YongGoose and others added 3 commits May 27, 2025 22:30

Implement configurable ReviewDog output directory

5ae21df

Signed-off-by: yongjunhong <yongjunh@apache.org>

Improve Gradle caching by using String lists instead of FormatterStep…

a7d515c

… objects Signed-off-by: yongjunhong <yongjunh@apache.org>

Merge branch 'main' into feature/655

376eafb

YongGoose requested a review from nedtwigg May 28, 2025 00:26

Merge branch 'diffplug:main' into feature/655

0cac676

	@TaskAction
	public void performAction(InputChanges inputs) throws Exception {
	IdeHook.State ideHook = getIdeHookState().getOrNull();
	if (ideHook != null && ideHook.path != null) {
	IdeHook.performHook(this, ideHook);
	return;
	}

	SpotlessTaskService taskService = getTaskService().get();
	taskService.registerSourceAlreadyRan(this);
	if (target == null) {
	throw new GradleException("You must specify 'Iterable<File> target'");
	}

	if (!inputs.isIncremental()) {
	getLogger().info("Not incremental: removing prior outputs");
	getFs().delete(d -> d.delete(cleanDirectory));
	getFs().delete(d -> d.delete(lintsDirectory));
	Files.createDirectories(cleanDirectory.toPath());
	Files.createDirectories(lintsDirectory.toPath());
	}

	private void performAction(boolean isTest) throws IOException {
	ConfigurableFileTree cleanFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessCleanDirectory().get());
	ConfigurableFileTree lintsFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessLintsDirectory().get());
	if (cleanFiles.isEmpty() && lintsFiles.isEmpty()) {
	getState().setDidWork(sourceDidWork());
	} else if (!isTest && applyHasRun()) {
	// if our matching apply has already run, then we don't need to do anything
	getState().setDidWork(false);
	} else {
	List<File> unformattedFiles = getUncleanFiles(cleanFiles);

	public void performAction() {
	getTaskService().get().registerApplyAlreadyRan(this);
	ConfigurableFileTree cleanFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessCleanDirectory().get());
	ConfigurableFileTree lintsFiles = getConfigCacheWorkaround().fileTree().from(getSpotlessLintsDirectory().get());
	if (cleanFiles.isEmpty() && lintsFiles.isEmpty()) {
	getState().setDidWork(sourceDidWork());
	} else {

	List<File> unformattedFiles = getUncleanFiles(cleanFiles);
	if (!unformattedFiles.isEmpty()) {
	// if any files are unformatted, we show those
	throw new GradleException(DiffMessageFormatter.builder()
	.runToFix(getRunToFixMessage().get())
	.formatterFolder(
	getProjectDir().get().getAsFile().toPath(),
	getSpotlessCleanDirectory().get().toPath(),
	getEncoding().get())
	.problemFiles(unformattedFiles)
	.getMessage());
	} else {
	// We only show lints if there are no unformatted files.
	// This is because lint line numbers are relative to the
	// formatted content, and formatting often fixes lints.
	boolean detailed = false;
	throw new GradleException(super.allLintsErrorMsgDetailed(lintsFiles, detailed));
	}

	TaskProvider<SpotlessCheck> checkTask = tasks.register(taskName + CHECK, SpotlessCheck.class, task -> {
	SpotlessTaskImpl source = spotlessTask.get();
	task.setGroup(TASK_GROUP);
	task.init(source);
	task.setEnabled(ideHook.path == null);
	task.dependsOn(source);

feat: Implement conversion of diff content to ReviewDog format #2478

Are you sure you want to change the base?

feat: Implement conversion of diff content to ReviewDog format #2478

Uh oh!

Conversation

YongGoose commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YongGoose commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YongGoose commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg commented May 21, 2025

Uh oh!

YongGoose commented May 21, 2025

Uh oh!

YongGoose commented May 21, 2025

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YongGoose commented May 21, 2025

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YongGoose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YongGoose commented Jun 1, 2025

Uh oh!

Uh oh!

YongGoose commented May 9, 2025 •

edited

Loading

YongGoose commented May 11, 2025 •

edited

Loading

nedtwigg commented May 12, 2025 •

edited

Loading

YongGoose commented May 16, 2025 •

edited

Loading