feat: Implement conversion of diff content to ReviewDog format by YongGoose · Pull Request #2478 · diffplug/spotless ·

Related issue: #655

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

@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

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 {

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

@nedtwigg

@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?

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.

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?

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.

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. 😁

@nedtwigg

@nedtwigg

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

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 <[email protected]>

Signed-off-by: yongjunhong <[email protected]>

@nedtwigg

@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 :)

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

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 <[email protected]>

@nedtwigg

@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
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 ⚡️

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
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 <[email protected]>

… objects Signed-off-by: yongjunhong <[email protected]>

@nedtwigg

@nedtwigg

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

Any updates?

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 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 <[email protected]>

Apply spotless

62eb863
Signed-off-by: yongjunhong <[email protected]>

Update README.md

e7015df
Signed-off-by: yongjunhong <[email protected]>

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 <[email protected]>

Add reviewDog property and pipe it to spotlessCheck

6688b33
Signed-off-by: yongjunhong <[email protected]>

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 <[email protected]>

Improve Gradle caching by using String lists instead of FormatterStep…

a7d515c
… objects Signed-off-by: yongjunhong <[email protected]>

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);

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!