spotlessCheck could add comments and suggested fix to GitHub PR

@rompic

Per @rompic's great idea on gitter.

https://github.com/jenkinsci/violation-comments-to-github-plugin

Violation-comments to github uses https://github.com/tomasbjerre/violations-lib
Which says in the read me:
Missing a format? Open an issue here!

I may also be able to have a look in a few days/weeks

Or you can add an option in Spotless to create a report on one of the formats supported by violations-lib.

Integration with https://github.com/reviewdog/reviewdog would also be nice

The reviewdog diagnostic format is designed to associate diagnostic messages with a code and a severity against sections of files. Correct me if I've misunderstood but I don't think Spotless has a variety of messages / codes / severities to offer. We'd be left to identify the suggested changes and produce uniform generic message / code / severity for each of them.

It appears that reviewdog also supports a diff input and given that Spotless is built around a simple Function<String, String> model, I wonder whether this might be a more appropriate integration point, producing a .diff file of changes (possibly reusing some diff logic from jgit), and leaving reviewdog to process this into PR comments etc.

The reviewdog data format includes a suggestions with replacementText field, which can accomplish the same things as the diff format. IMO it's a bit easier to generate than .patch, because you don't have to futz with the context lines which allow normal patches to be applied even if the line numbers change.

I agree that the messages/codes/severities will all be the same, except for failures. Spotless is designed around Function<String, String>, but sometimes a formatter will instead throw an Exception (which we consider to be an internal failure) or an Error (which we consider to be an intentional error message for the user).

In particular, ktlint throws errors for lots of things which don't fit the Function<String, String> model. I'm not a fan of linters of this type, but some people are, and it would be nice for Spotless to support them.

If you make a PR which adds a spotlessDiff task, I'd be happy to merge it, but I'm not sure what the correct behavior should be if a formatter errors out on some specific files. IMO, it is just as easy to integrate with the reviewdog format as the diff format, but with the added simplicity that you can always generate a complete report even for formatters which throw lots of errors like the very popular ktlint.

A PR against #1097 which pipes Lint.java into either https://github.com/Contrast-Security-OSS/java-sarif (add it to lib-extra) or any other reviewdog format which supports "suggestions" would be very welcome. We need to figure out which fields Lint needs, and that's a tug of war between

what do the linters have to give
what do sarif and reviewdog want

@nedtwigg

@nedtwigg

I'm interested in this feature, so I'm planning to implement it myself. 🚀
Before jumping into the implementation, I'd like to understand the current status of the issue so I can design the solution properly.

To elaborate a bit more: this feature has been discussed across several issues and discussions.

GitHub Action that translates spotless apply diff into GitHub pull request code suggestion comments #1704
Provide a GitHub Action that translates spotless apply diff into GitHub pull request code suggestion comments #1700

One of the related issues mentioned that the feature was blocked due to internal changes, but looking at the PR, it seems that it has since been merged.

I also noticed another PR that introduces the concept of lint, which seems related to solving this issue.

Linting - API #2149

So, to ensure a smooth design process, I'd like to confirm the latest state of the issue.

I'm interested in this feature, so I'm planning to implement it myself. 🚀

Wonderful!!!

the feature was blocked due to internal changes, but looking at the PR, it seems that it has since been merged.

Correct, the blockers to this have been solved. Our core abstractions are now very stable, I anticipate no changes to our core abstractions in the foreseeable future.

Big picture, the goal of Spotless is to be a formatter, not a linter. But inevitably, there are some "formatting issues" which can't be automatically fixed. Syntax errors, for example. And there are linters which can sometimes automatically provide fixes, which makes them more like a formatter. So the line is blurrier than it first appears.

Spotless handles this like so:

class DirtyState {
  boolean isClean()
  byte[] canonicalBytes() // only legal to call if !isClean()
}
class Lint {
  int lineStart, lineEnd; //1-indexed, inclusive
  String shortCode, detail;
}
class LintState {
  DirtyState dirtyState;
  List<List<Lint>> lintsPerStep;
}
// key entrypoint
public static LintState of(Formatter formatter, File file) { ... }

So inside a spotlessCheck, you have the LintState of each file, which tells you two separate things:

the LintState.dirtyState gives you the clean state of the file. You'll have to diff it yourself to create the hunks that can automatically move the code towards its formatted state

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/integration/DiffMessageFormatter.java

Lines 278 to 281 in d97b95d

    
           RawText a = new RawText(dirty.getBytes(StandardCharsets.UTF_8)); 
        
           RawText b = new RawText(clean.getBytes(StandardCharsets.UTF_8)); 
        
           EditList edits = new EditList(); 
        
           edits.addAll(MyersDiff.INSTANCE.diff(RawTextComparator.DEFAULT, a, b));

the LintState.lintsPerStep give you the things which could not be fixed automatically. Syntax errors, maybe a rule that says "don't import lib.internal.*".

key issue these lints are relative to the clean version
Spotless always does idempotency fixing. It's possible that in the original version, a lint exists which is resolved by the formatting. And it's possible that the final formatted version creates a lint from the perspective of one of the steps.
So we actually can't meaningfully show the lints until the formatting has happened

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

Lines 69 to 85 in d97b95d

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

It seems more complicated than it needs to be. But until we separated out this lint concept, it was common for silly little quibbles to prevent a file from being formatted. Now the file can get formatted first, and then we fail the build based on the quibble.

If you run spotlessApply, you only get one failure - the formatting is fixed automatically and then it yells at you about the lints. But if you're using spotlessCheck to generate PR comments, there is some jankiness in that you will first get yelled at because of formatting, and then you'll come back and get yelled at about lints. I think it's okay, because there aren't many lints - the point of Spotless is to just fix stuff! But the "pure formatter" abstraction is a bit leaky, and this is the fallout.

to ensure a smooth design process, I'd like to confirm the latest state of the issue

I have no strong opinion that it needs to use or avoid ReviewDog vs some other middleware vs go straight through hub4j/github-api. Implementor's choice. If one gets built I think it will be relatively easy for people to integrate with it in other ways later.

This isn't required, but I think snapshot testing will be quite helpful. We are already using Selfie for inline snapshots like so:

spotless/testlib/src/test/java/com/diffplug/spotless/java/GoogleJavaFormatStepTest.java

Lines 69 to 70 in d97b95d

    
           .expectLintsOfResource("java/googlejavaformat/JavaCodeWithLicenseUnformatted.test") 
        
           .toBe("LINE_UNDEFINED google-java-format(jvm-version) You are running Spotless on JVM 21. This requires google-java-format of at least 1.17.0 (you are using 1.2). (...)");

Thank you for your detailed explanation :)

I’ll try designing the solution myself first, and then share a draft PR (or something similar) to get your feedback.
I look forward to collaborating with you on this task!

@YongGoose

awesome feature @YongGoose !

feat: Implement conversion of diff content to ReviewDog format

@nedtwigg

@nedtwigg

This is my first time contributing to Spotless, and I’m still figuring things out — so I’ve opened a Draft PR for now. 🥹

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

My plan is to make SpotlessDiff return diffs in the ReviewDog format.

Since the output would follow the ReviewDog format, it might no longer align with the name SpotlessDiff — so I think this is something we should discuss.

Then, a separate step in the workflow will take care of passing those diffs to ReviewDog, which will comment on the PR accordingly.

There’s still a lot to improve, and I’m aware that the current version needs a lot of work.
I’d really appreciate any feedback, and I’d love to keep building this together. 💪🏻

Has there been any progress toward this? I saw that #2478 been closed.

@YongGoose

@YongGoose did a great job, but I don't have time to mentor this at a fine level of detail. The work he put in at #2478 is a great starting point.

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	RawText a = new RawText(dirty.getBytes(StandardCharsets.UTF_8));
	RawText b = new RawText(clean.getBytes(StandardCharsets.UTF_8));
	EditList edits = new EditList();
	edits.addAll(MyersDiff.INSTANCE.diff(RawTextComparator.DEFAULT, a, b));

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

	.expectLintsOfResource("java/googlejavaformat/JavaCodeWithLicenseUnformatted.test")
	.toBe("LINE_UNDEFINED google-java-format(jvm-version) You are running Spotless on JVM 21. This requires google-java-format of at least 1.17.0 (you are using 1.2). (...)");

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

Description

Activity

rompic commented on Jul 30, 2020

tomasbjerre commented on Jul 31, 2020

Meemaw commented on Sep 7, 2020

roxspring commented on Mar 6, 2021

nedtwigg commented on Mar 6, 2021

nedtwigg commented on Jan 14, 2022

YongGoose commented on May 5, 2025

nedtwigg commented on May 6, 2025

YongGoose commented on May 6, 2025

injae-kim commented on May 9, 2025

YongGoose commented on May 9, 2025

NicklasWallgren commented on Sep 21, 2025

nedtwigg commented on Sep 24, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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