Merge pull request #455 from diffplug/feature/fix-padded-cell

Fix padded cell

CHANGES.md

@@ -11,6 +11,12 @@ You might be looking for:
   * Fixes [#410](https://github.com/diffplug/spotless/issues/410) AccessDeniedException in MinGW/ GitBash.
   * Also fixes occasional [hang on NFS due to filesystem timers](https://github.com/diffplug/spotless/pull/407#issuecomment-514824364).
 * Eclipse-based formatters used to leave temporary files around ([#447](https://github.com/diffplug/spotless/issues/447)). This is now fixed, but only for eclipse 4.12+, no back-port to older Eclipse formatter versions is planned. ([#451](https://github.com/diffplug/spotless/issues/451))
+* `PaddedCellBulk` had a bug where badly-formatted files with well-behaving formatters were being:
+    - correctly formatted by "apply"
+    - but incorrectly marked as good by "check"
+    - this led to "check" says all good, but then "apply" still causes format (https://github.com/diffplug/spotless/issues/453)
+    - combined with up-to-date checking, could lead to even more confusing results (https://github.com/diffplug/spotless/issues/338)
+    - only affects the gradle plugin, since that was the only plugin to use this feature
 * Minor change to `TestProvisioner`, which should fix the cache-breaking issues, allowing us to speed-up the CI builds a bit.
 
 ### Version 1.24.1 - August 12th 2018 (javadoc [lib](https://diffplug.github.io/spotless/javadoc/spotless-lib/1.24.1/) [lib-extra](https://diffplug.github.io/spotless/javadoc/spotless-lib-extra/1.24.1/), artifact [lib]([jcenter](https://bintray.com/diffplug/opensource/spotless-lib), [lib-extra]([jcenter](https://bintray.com/diffplug/opensource/spotless-lib-extra)))

build.gradle

@@ -2,7 +2,7 @@ plugins {
 	// dependencyUpdates https://github.com/ben-manes/gradle-versions-plugin/releases
 	id 'com.github.ben-manes.versions' version '0.22.0'
 	// https://github.com/diffplug/goomph/blob/master/CHANGES.md
-	id 'com.diffplug.gradle.eclipse.resourcefilters' version '3.18.0'
+	id 'com.diffplug.gradle.eclipse.resourcefilters' version '3.18.1'
 	// https://plugins.gradle.org/plugin/com.gradle.plugin-publish
 	id 'com.gradle.plugin-publish' version '0.10.1' apply false
 	// https://plugins.gradle.org/plugin/com.gradle.build-scan

ide/build.gradle

@@ -12,6 +12,6 @@ oomphIde {
 
 	thirdParty {
 		minimalistGradleEditor {}
-		tmTerminal {}
+		//tmTerminal {}
 	}
 }

lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java

@@ -143,18 +143,18 @@ public static List<File> check(File rootDir, File diagnoseDir, Formatter formatt
 						Files.write(path, version.getBytes(formatter.getEncoding()));
 					}
 					// dump the type of the misbehavior to console
-					logger.finer("    " + relative + " " + padded.userMessage());
+					logger.fine("    " + relative + " " + padded.userMessage());
+				}
 
-					if (!padded.isResolvable()) {
-						// if it's not resolvable, then there's
-						// no point killing the build over it
-					} else {
-						// if the input is resolvable, we'll use that to try again at
-						// determining if it's clean
-						paddedCellStep.set(problemFile, padded.canonical());
-						if (!paddedFormatter.isClean(problemFile)) {
-							stillFailing.add(problemFile);
-						}
+				if (!padded.isResolvable()) {
+					// if it's not resolvable, then there's
+					// no point killing the build over it
+				} else {
+					// if the input is resolvable, we'll use that to try again at
+					// determining if it's clean
+					paddedCellStep.set(problemFile, padded.canonical());
+					if (!paddedFormatter.isClean(problemFile)) {
+						stillFailing.add(problemFile);
 					}
 				}
 			}

plugin-gradle/CHANGES.md

@@ -6,6 +6,13 @@
   * Fixes [#410](https://github.com/diffplug/spotless/issues/410) AccessDeniedException in MinGW/GitBash.
   * Also fixes occasional [hang on NFS due to filesystem timers](https://github.com/diffplug/spotless/pull/407#issuecomment-514824364).
 * Eclipse-based formatters used to leave temporary files around ([#447](https://github.com/diffplug/spotless/issues/447)). This is now fixed, but only for eclipse 4.12+, no back-port to older Eclipse formatter versions is planned. ([#451](https://github.com/diffplug/spotless/issues/451))
+* Fixed a bad but simple bug in `paddedCell()` (https://github.com/diffplug/spotless/pull/455)
+    - if a formatter was behaving correctly on a given file (was idempotent)
+    - but the file was not properly formatted
+    - `spotlessCheck` would improperly say "all good" even though `spotlessApply` would properly change them
+    - combined with up-to-date checking, could lead to even more confusing results,
+     (https://github.com/diffplug/spotless/issues/338)
+    - Fixed now!
 
 ### Version 3.24.2 - August 19th 2019 ([javadoc](https://diffplug.github.io/spotless/javadoc/spotless-plugin-gradle/3.24.1/), [jcenter](https://bintray.com/diffplug/opensource/spotless-plugin-gradle/3.24.1))
 

plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java

@@ -90,6 +90,10 @@ private String checkFailureMsg() {
 		}
 	}
 
+	private Bundle wellbehaved() throws IOException {
+		return new Bundle("wellbehaved", x -> "42");
+	}
+
 	private Bundle cycle() throws IOException {
 		return new Bundle("cycle", x -> x.equals("A") ? "B" : "A");
 	}
@@ -104,32 +108,38 @@ private Bundle diverge() throws IOException {
 
 	@Test
 	public void failsWithoutPaddedCell() throws IOException {
+		Assertions.assertThat(wellbehaved().checkFailureMsg()).startsWith("The following files had format violations");
 		Assertions.assertThat(cycle().checkFailureMsg()).startsWith("You have a misbehaving rule");
 		Assertions.assertThat(converge().checkFailureMsg()).startsWith("You have a misbehaving rule");
 		Assertions.assertThat(diverge().checkFailureMsg()).startsWith("You have a misbehaving rule");
 	}
 
 	@Test
 	public void paddedCellApply() throws Exception {
+		Bundle wellbehaved = wellbehaved().paddedCell();
 		Bundle cycle = cycle().paddedCell();
 		Bundle converge = converge().paddedCell();
 		Bundle diverge = diverge().paddedCell();
 
+		execute(wellbehaved.apply);
 		execute(cycle.apply);
 		execute(converge.apply);
 		execute(diverge.apply);
 
+		assertFile(wellbehaved.file).hasContent("42");	// cycle -> first element in cycle
 		assertFile(cycle.file).hasContent("A");		// cycle -> first element in cycle
 		assertFile(converge.file).hasContent("");	// converge -> converges
 		assertFile(diverge.file).hasContent("CCC");	// diverge -> no change
 
+		execute(wellbehaved.check);
 		execute(cycle.check);
 		execute(converge.check);
 		execute(diverge.check);
 	}
 
 	@Test
 	public void paddedCellCheckFailureFiles() throws Exception {
+		wellbehaved().paddedCell().checkFailureMsg();
 		cycle().paddedCell().checkFailureMsg();
 		converge().paddedCell().checkFailureMsg();
 		execute(diverge().paddedCell().check);