Add an ignoreFailures property

[andrewe123]

Like pmd, checkstyles, findbugs plugins.

when ignoreFailures = true the build doesn't fail.

spotless {
   ignoreFailures = true
  ...
}

[jbduncan]

I admit I'm personally reluctant to have this added, on one hand because I see Spotless as a sort of absolute style checker/enforcer, and on the other hand because the Eclipse formatter allows chunks of code between an // @formatter:off and an // @formatter:on to be ignored.

@andrewe123 Can you explain to us your reason(s) for wanting this functionality, and whether existing options like the Eclipse formatter's // @formatter:{off|on} comment-annotations meet your needs or not?

[andrewe123]

My use-case is a common checks plugin with a standardized formatting template for multiple Java repositories, where the team have the flexibility to enable or disable Spotless on different pipelines.

[nedtwigg]

I think it's a reasonable use-case, but interestingly I don't think we can build it without hurting performance in a major way! Here's why:

1. The first time Spotless runs, it runs on all files.
2. If Spotless succeeded, it uses gradle's incremental build support to remember which files are formatted properly.
3. The second time Spotless runs, it runs only on new or changed files. Spotless knows that all the other files are formatted correctly, so there's no need to rerun on them.

If we built support for ignoreFailures, failures would only be reported once - the very first time they are encountered. After that, the incremental build cache will assume that they are okay, since the previous build passed, and so Spotless won't warn about them again in the future.

In order to support this flag, we will have to disable incremental build. Spotless is pretty fast even without incremental build, so it still makes some sense to implement this.

Changing this snippet to pass either all files or just the changed files based on the value of the ignoreFailures flag is pretty easy:

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

Lines 157 to 163 in 23de01a

	List<File> outOfDate = new ArrayList<>();
	inputs.outOfDate(inputDetails -> {
	File file = inputDetails.getFile();
	if (file.isFile()) {
	outOfDate.add(file);
	}
	});

But we'd also need a way to force the task to run even if absolutely nothing has changed. One way is to add a dummy input property to the task which is never equal to itself, but there might be others.

[andrewe123]

@nedtwigg Thanks.

I'm planning to go with adding a project property around applying Spotless in the plugin for now.

So developers can enable it by doing something like this.

gradle check -PenableSpotless

If Spotless succeeded, it uses gradle's incremental build support to remember which files are formatted properly.

I'm not fully familiar with how the incremental build works. Is it safe to assume if we always use clean on our pipelines, to remove class files, we won't have any issues?

[nedtwigg]

You won't have any issues whether you use clean or not.

The incremental build remembers the exact state of the formatter, including any configuration files, and the exact state of the files on their last successful build. So you can disable spotless for a week, and turn it on again a week later. Probably the gradle cache will have evicted Spotless' state by then, and it will run on all files. But even if the cache hasn't evicted Spotless, then it will make sure the Spotless config hasn't changed (including config files), and either run on all files, or just the files that changed during that week, as appropriate.

[ghost]

How can I ignore spotless in some submodule?
for example we are using jooq to generate db classes (they are not pushed to repo) and I would like to skip spotless check on this submodule.

I see there is ignoreErrorForPath but I don't know how to use it, the path should be relative to what?

[nedtwigg]

@bassmake Take a look at the target parameter here.

[gdecaso]

I would also like to see a way to configure when and if to fail. In my opinion, spotless should only add the spotlessCheck and spotlessApply tasks without making the check task depend on spotlessCheck.

A few use cases:

• building while working on a uncommitted changes should not require them to be fully formatted.
• building a feature branch in a CI pipeline should also work even if there are formatting failures
• pipelines for PRs may decide to check the format at this point and fail accordingly
• pipelines for master changesets should definitely fail if there are formatting errors

To sum up, whether to fail or not a build based on format is highly dependent on context and programmers preferences/styles. If this plugin does not support configuring that then it will force its users to hack (e.g., spotelessCheck.dependsOn.remove('check') and such) or reduce its target audience.

To be more precise, I'd simply remove this full block in L106-108 https://github.com/diffplug/spotless/blob/master/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessPlugin.java#L108

Happy to create a PR if you think that helps

Just my 2 cents

[nedtwigg]

Formatting is not important. The point of Spotless is to take that discussion off the table - this is how this project is formatted, period, so let's talk about the content instead of the formatting.

You're looking at Spotless, so you've decided that you'd like for your code to be formatted a specific way.

You're opening a new discussion - when should my code be formatted the way I want it to be formatted? to which there are three possible answers:

1. Never (if you're using Spotless, then that's not what you want)
2. Sometimes
3. Always

Always is simple. Formatting is not important, so I think the simple answer is the best answer, and the right default for Spotless.

Sometimes is complicated. If you have a compelling need to implement your particular Sometimes policy, Spotless supports that with spotlessCheck.dependsOn.remove('check') EDIT NedTwigg June 30 2020

spotless {
    enforceCheck = false
}

[gdecaso]

@nedtwigg thanks for the quick reply!

I agree that formatting is not important. Let me re-state my point in case it was not clear. I am not against formatting always. I am against checking the formatting always.

In other words, if the plugin was such that Java's check dependsOn spotlessApply then I'd have no objections. However, checking on every build is forcing an unnatural step to have to go and fix formatting (even if it is via the spotlessApply Gradle task) which goes in the exact opposite direction that you are claiming as it is making formatting top-of-mind for the poor committed who just wants her tests to run

In any case, let me fork and see if I can find a way to make this dependency configurable

[gdecaso]

Created PR #95. Thx

[nedtwigg]

I am not against formatting always. I am against checking the formatting always.

If you format always, then the check will always pass :)

if the plugin was such that Java's check dependsOn spotlessApply

Then I could commit code with formatting errors, upload it to CI, and it would pass CI. And the commit history would be hard to read, because there would be meaningless formatting changes obscuring the changes that matter.

This commit has 16 changed lines. 2 of them matter, the other 14 are formatting noise which makes it harder to review the content of the change. If check depends on spotlessApply, CI cannot catch this.

[nedtwigg]

Now available in 3.2.0-SNAPSHOT:

spotless {
    enforceCheck = false
}

[nedtwigg]

Thanks for the PR! It has been published as 3.2.0. Docs available here.