diffplug
warn user when a file being a part of multiple formats
Reading https://github.com/diffplug/spotless/blob/main/PADDEDCELL.md, I may have some expectations regarding idempotency which may not be true (at least with the mvn plugin).
I consider a configuration like the following:
<configuration>
<formats>
<format>
<!-- define the files to apply to -->
<includes>
<include>.gitignore</include>
<include>*.md</include>
</includes>
<!-- define the steps to apply to those files -->
<trimTrailingWhitespace />
<endWithNewline />
<indent>
<tabs>true</tabs>
<spacesPerTab>4</spacesPerTab>
</indent>
</format>
</formats>
<markdown>
<includes>
<!-- You have to set the target manually -->
<include>*.md</include>
<include>src/**/*.md</include>
</includes>
<flexmark />
</markdown>
</configuration>
We see *.MD files are caught by both formats and markdown: this is certainly some left-overs on my side, but I would expect Spotless to behave consistently anyway (or to throw alltogether).
On mvn spotless:apply:
[INFO] --- spotless-maven-plugin:2.34.0:apply (default-cli) @ monolith ---
[INFO] Writing clean file: /Users/blacelle/workspace3/mitrust-datasharing/exec/monolith/README.md
[INFO] Spotless.Markdown is keeping 1 files clean - 1 were changed to be clean, 0 were already clean, 0 were skipped because caching determined they were already clean
[INFO] Writing clean file: /Users/blacelle/workspace3/mitrust-datasharing/exec/monolith/README.md
[INFO] Spotless.Format is keeping 1 files clean - 1 were changed to be clean, 0 were already clean, 0 were skipped because caching determined they were already clean
-> The files are written twice: I suppose we have conflicting indentations, and we stop after a cycle: fine.
However, mvn spotless:check complains about the style of my files: I would either expect apply to fail due to a cycle (or a bad configuration as a single file is covered by multiple languages), or check to succeed (potentially with a warning.
Is this a bug or a limitation ? If given (sub-optimal, if not invalid) configuration is considered valid, https://github.com/diffplug/spotless/pull/1643 would help having a consistent behavior.
I think it is a user bug if a file is specified as a target by multiple formats. But it is a user bug which we should help with!
When a spotless:check run fails, it is okay to spend a lot of CPU time to help the user figure out why. I think we should do something like
for each failed file
for each format except ourselves
iterate over target, if target contains this failed file warn the user
This is applicable to the Gradle and Maven plugins, but the implementation of the feature is quite specific to each plugin.
I would categorize and retitle this as "enhancement: warn user when a failure might be caused by a file being a part of multiple formats", does that agree with you?
#1653 demonstrate another unexpected behavior with such faulty configuration: both spotless:apply and spotless:check telling SUCCESS but not modifying files requiring formatting.
With Gradle, overlap checking only during a failed check is sufficient. This is because file caching happens per-format. If A and B are both formatting the same file, they will both cache their own output version, and you'll see failures if A and B disagree.
With Maven, I think the cache is global across all formats, so the cache stores only the output of either A or B in a haphazard way which hides the overlapping target. @lutovich just FYI.
We could do overlap checking during index creation, but that incurs quite a cost on even well-functioning builds. I think a better approach is for the up-to-date index to store outputs per-format-per-file, so that disagreeing formats with overlap will cause a failure. Then we can check for overlap only during check failure.
We are actually in a similar boat as https://github.com/diffplug/spotless/issues/1767 user. The difference is that we use <java> (Google Style) and <format> for all Java files as a complimentary rules which Google format doesn't cover. See the following example:
<configuration>
<lineEndings>UNIX</lineEndings>
<java>
<includes>
<include>src/**/*.java</include>
</includes>
<googleJavaFormat>
<reflowLongStrings>true</reflowLongStrings>
</googleJavaFormat>
<formatAnnotations/>
</java>
<formats>
<format>
<includes>
<include>src/**/*.java</include>
</includes>
<replaceRegex>
<name>Blank line at the start of a block is not allowed</name>
<searchRegex>(\{\n)\s*\n</searchRegex>
<replacement>$1</replacement>
</replaceRegex>
<replaceRegex>
<name>Blank line at the end of a block is not allowed</name>
<searchRegex>(\n)\s*\n([ ]*\}\n)</searchRegex>
<replacement>$1$2</replacement>
</replaceRegex>
<replaceRegex>
<name>Consecutive blank lines are not allowed</name>
<searchRegex>(\n\n)\n+</searchRegex>
<replacement>$1</replacement>
</replaceRegex>
</format>
</formats>
</configuration>
This worked fine for pre-2.35 version, this also works fine if we disable up-to-date index:
<upToDateChecking>
<enabled>false</enabled>
</upToDateChecking>
So this warning will be of no use for us, or it should link to a very clear documentation. At the moment, I'm completely lost how to solve this correctly.
@ViliusS you can put all of that into one format. The java format has access to everything in the generic format.
<configuration>
<lineEndings>UNIX</lineEndings>
<java>
<includes>
<include>src/**/*.java</include>
</includes>
<googleJavaFormat>
<reflowLongStrings>true</reflowLongStrings>
</googleJavaFormat>
<formatAnnotations/>
<replaceRegex>
<name>Blank line at the start of a block is not allowed</name>
<searchRegex>(\{\n)\s*\n</searchRegex>
<replacement>$1</replacement>
</replaceRegex>
<replaceRegex>
<name>Blank line at the end of a block is not allowed</name>
<searchRegex>(\n)\s*\n([ ]*\}\n)</searchRegex>
<replacement>$1$2</replacement>
</replaceRegex>
<replaceRegex>
<name>Consecutive blank lines are not allowed</name>
<searchRegex>(\n\n)\n+</searchRegex>
<replacement>$1</replacement>
</replaceRegex>
</java>
</configuration>
OK, did know that was possible. I guess documentation could be more clear on this too. In a README table replaceRegex is shown as a separate generic step generic.ReplaceRegexStep, which I assumed is only possible as a separate option.
Just wondering, do other formats, like WPT Eclipse or sortPom also have access to all generic formatting?
Yes
https://github.com/diffplug/spotless/blob/657926464aae27dee2c83c5b28ac6b7e52f2229a/plugin-maven/src/main/java/com/diffplug/spotless/maven/FormatterFactory.java#L136-L146
https://github.com/diffplug/spotless/blob/657926464aae27dee2c83c5b28ac6b7e52f2229a/plugin-maven/src/main/java/com/diffplug/spotless/maven/java/Java.java#L39