spotlessCheck could add comments and suggested fix to GitHub PR

[nedtwigg]

Per @rompic's great idea on gitter.

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

[rompic]

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

[tomasbjerre]

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

[Meemaw]

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

[roxspring]

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.

[nedtwigg]

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.

[nedtwigg]

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

[YongGoose]

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

[nedtwigg]

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). (...)");

[YongGoose]

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!