You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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?
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.
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:
The first time Spotless runs, it runs on all files.
If Spotless succeeded, it uses gradle's incremental build support to remember which files are formatted properly.
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:
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.
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?
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.
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.
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:
Never (if you're using Spotless, then that's not what you want)
Sometimes
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
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
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.
Activity
jbduncan commentedon Feb 22, 2017
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:offand an// @formatter:onto 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 commentedon Feb 22, 2017
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 commentedon Feb 22, 2017
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:
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
ignoreFailuresflag is pretty easy:spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java
Lines 157 to 163 in 23de01a
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 commentedon Feb 23, 2017
@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.
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 commentedon Feb 23, 2017
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.
nedtwigg commentedon Mar 21, 2017
@bassmake Take a look at the
targetparameter here.gdecaso commentedon Mar 31, 2017
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:
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 commentedon Mar 31, 2017
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:
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')gdecaso commentedon Mar 31, 2017
@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 commentedon Mar 31, 2017
Created PR #95. Thx
nedtwigg commentedon Apr 1, 2017
If you format always, then the check will always pass :)
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
checkdepends onspotlessApply, CI cannot catch this.nedtwigg commentedon Apr 3, 2017
Now available in 3.2.0-SNAPSHOT:
nedtwigg commentedon Apr 3, 2017
Thanks for the PR! It has been published as 3.2.0. Docs available here.