Merge pull request #634 from diffplug/feat/remove-questionable-defaults

nedtwigg · web-flow · commit fd07b202e07e · 2020-07-01T21:19:29.000-07:00
Remove questionable default targets
diff --git a/lib/src/main/java/com/diffplug/spotless/cpp/CppDefaults.java b/lib/src/main/java/com/diffplug/spotless/cpp/CppDefaults.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2020 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@ public class CppDefaults {
 	 * Filter based on Eclipse-CDT <code>org.eclipse.core.contenttype.contentTypes</code>
 	 * extension <code>cSource</code>, <code>cHeader</code>, <code>cxxSource</code> and <code>cxxHeader</code>.
 	 */
+	@Deprecated
 	public static final List<String> FILE_FILTER = Collections.unmodifiableList(
 			Arrays.asList("c", "h", "C", "cpp", "cxx", "cc", "c++", "h", "hpp", "hh", "hxx", "inc")
 					.stream().map(s -> {
diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md
@@ -13,6 +13,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 ### Fixed
 * LineEndings.GIT_ATTRIBUTES is now a bit more efficient, and paves the way for remote build cache support in Gradle. ([#621](https://github.com/diffplug/spotless/pull/621))
 * `ratchetFrom` now ratchets from the merge base of `HEAD` and the specified branch.  This fixes the surprising behavior when a remote branch advanced ([#631](https://github.com/diffplug/spotless/pull/631) fixes [#627](https://github.com/diffplug/spotless/issues/627)).
+### Deprecated
+* The default targets for `C/C++`, `freshmark`, `sql`, and `typescript` now generate a warning, asking the user to specify a target manually. There is no well-established convention for these languages in the gradle ecosystem, and the performance of the default target is far worse than a user-provided one.  If you dislike this change, please complain in [#634](https://github.com/diffplug/spotless/pull/634).
 
 ## [4.4.0] - 2020-06-19
 ### Added
diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/CppExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/CppExtension.java
@@ -65,6 +65,7 @@ protected void setupTask(SpotlessTask task) {
 			 * Hence file extension based filtering is used in line with the org.eclipse.core.contenttype.contentTypes<
 			 * defined by the CDT plugin.
 			 */
+			noDefaultTarget();
 			target(CppDefaults.FILE_FILTER.toArray());
 		}
 		super.setupTask(task);
diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java
@@ -671,4 +671,8 @@ public SpotlessApply createIndependentApplyTask(String taskName) {
 
 		return applyTask;
 	}
+
+	protected void noDefaultTarget() {
+		getProject().getLogger().warn("Spotless: no target set for " + formatName() + ", will be an error in the next release!");
+	}
 }
diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FreshMarkExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FreshMarkExtension.java
@@ -62,6 +62,7 @@ public void propertiesFile(Object... files) {
 	protected void setupTask(SpotlessTask task) {
 		// defaults to all markdown files
 		if (target == null) {
+			noDefaultTarget();
 			target = parseTarget("**/*.md");
 		}
 		super.setupTask(task);
diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SqlExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SqlExtension.java
@@ -57,6 +57,7 @@ private FormatterStep createStep() {
 	protected void setupTask(SpotlessTask task) {
 		if (target == null) {
 			target("**/*.sql");
+			noDefaultTarget();
 		}
 		super.setupTask(task);
 	}
diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/TypescriptExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/TypescriptExtension.java
@@ -170,6 +170,7 @@ private void fixParserToTypescript() {
 	protected void setupTask(SpotlessTask task) {
 		// defaults to all typescript files
 		if (target == null) {
+			noDefaultTarget();
 			target = parseTarget("**/*.ts");
 		}
 		super.setupTask(task);
diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md
@@ -10,6 +10,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config` or `configFile` with the option `parser` or `filepath` ([#620](https://github.com/diffplug/spotless/pull/620)).
 * Added ANTLR4 support ([#326](https://github.com/diffplug/spotless/issues/326)).
 ### Removed
+* **BREAKING** the default includes for `<typescript>` and `<cpp>` were removed, and will now generate an error if an `<include>` is not specified.  There is no well-established convention for these languages in the maven ecosystem, and the performance of the default includes is far worse than a user-provided one.  If you dislike this change, please complain in [#634](https://github.com/diffplug/spotless/pull/634), it would not be a breaking change to bring the defaults back.
 * **BREAKING** the long-deprecated `<xml>` and `<css>` formats have been removed, in favor of the long-available [`<eclipseWtp>`](https://github.com/diffplug/spotless/tree/main/plugin-maven#eclipse-wtp) step which is available in every generic format.
   * This probably doesn't affect you, but if it does, you just need to change `<xml>...` into `<formats><format><eclipseWtp><type>XML</type>...`
   * In [`1.15.0` (released 2018-09-23)](#1150---2018-09-23), we added support for `xml` and `css` formats using the Eclipse WTP.
diff --git a/plugin-maven/README.md b/plugin-maven/README.md
@@ -294,8 +294,7 @@ Spotless requires Maven to be running on JRE 8+.
 ```xml
 <configuration>
   <cpp>
-    <!-- You have to set the target manually -->
-    <includes>
+    <includes> <!-- You have to set the target manually -->
       <include>src/native/**</inclue>
     </includes>
 
@@ -359,8 +358,7 @@ Spotless requires Maven to be running on JRE 8+.
 ```xml
 <configuration>
   <typescript>
-    <!-- These are the defaults, you can override if you want -->
-    <includes>
+    <includes> <!-- You have to set the target manually -->
       <include>src/**/*.ts</include>
     </includes>
 
diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java
@@ -148,6 +148,9 @@ private List<File> collectFiles(FormatterFactory formatterFactory) throws MojoEx
 		Set<String> configuredExcludes = formatterFactory.excludes();
 
 		Set<String> includes = configuredIncludes.isEmpty() ? formatterFactory.defaultIncludes() : configuredIncludes;
+		if (includes.isEmpty()) {
+			throw new MojoExecutionException("You must specify some files to include, such as '<includes><include>src/**</include></includes>'");
+		}
 
 		Set<String> excludes = new HashSet<>(FileUtils.getDefaultExcludesAsList());
 		excludes.add(withTrailingSeparator(buildDir.toString()));
diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/cpp/Cpp.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/cpp/Cpp.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2020 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,10 +15,8 @@
  */
 package com.diffplug.spotless.maven.cpp;
 
-import java.util.Arrays;
-import java.util.Collection;
+import java.util.Collections;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 import com.diffplug.spotless.cpp.CppDefaults;
 import com.diffplug.spotless.maven.FormatterFactory;
@@ -31,15 +29,9 @@
  * and cpp-specific (e.g. {@link Eclipse}) steps.
  */
 public class Cpp extends FormatterFactory {
-
-	private static final Set<String> DEFAULT_INCLUDES = CppDefaults.FILE_FILTER
-			.stream().map(s -> {
-				return Arrays.asList("src/main/cpp/" + s, "src/test/cpp/" + s);
-			}).flatMap(Collection::stream).collect(Collectors.toSet());
-
 	@Override
 	public Set<String> defaultIncludes() {
-		return DEFAULT_INCLUDES;
+		return Collections.emptySet();
 	}
 
 	public void addEclipse(Eclipse eclipse) {
diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/typescript/Typescript.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/typescript/Typescript.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2020 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,9 +15,9 @@
  */
 package com.diffplug.spotless.maven.typescript;
 
+import java.util.Collections;
 import java.util.Set;
 
-import com.diffplug.common.collect.ImmutableSet;
 import com.diffplug.spotless.maven.FormatterFactory;
 
 /**
@@ -26,14 +26,11 @@
  * It defines a formatter for typescript source files.
  */
 public class Typescript extends FormatterFactory {
-
-	private static final Set<String> DEFAULT_INCLUDES = ImmutableSet.of("src/**/*.ts");
-
 	private static final String LICENSE_HEADER_DELIMITER = null;
 
 	@Override
 	public Set<String> defaultIncludes() {
-		return DEFAULT_INCLUDES;
+		return Collections.emptySet();
 	}
 
 	@Override
@@ -44,5 +41,4 @@ public String licenseHeaderDelimiter() {
 	public void addTsfmt(Tsfmt tsfmt) {
 		addStepFactory(tsfmt);
 	}
-
 }
diff --git a/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenIntegrationHarness.java b/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenIntegrationHarness.java
@@ -119,7 +119,7 @@ protected void writePomWithCppSteps(String... steps) throws IOException {
 	}
 
 	protected void writePomWithTypescriptSteps(String... steps) throws IOException {
-		writePom(groupWithSteps("typescript", steps));
+		writePom(groupWithSteps("typescript", including("**/*.ts"), steps));
 	}
 
 	protected void writePomWithPrettierSteps(String includes, String... steps) throws IOException {
diff --git a/plugin-maven/src/test/java/com/diffplug/spotless/maven/cpp/CppTest.java b/plugin-maven/src/test/java/com/diffplug/spotless/maven/cpp/CppTest.java
diff --git a/plugin-maven/src/test/java/com/diffplug/spotless/maven/generic/LicenseHeaderTest.java b/plugin-maven/src/test/java/com/diffplug/spotless/maven/generic/LicenseHeaderTest.java
@@ -37,6 +37,7 @@ public void fromFileJava() throws Exception {
 	public void fromContentCpp() throws Exception {
 		String cppLicense = "//my license";
 		writePomWithCppSteps(
+				"<includes><include>src/**</include></includes>",
 				"<licenseHeader>",
 				"  <content>",
 				cppLicense,

Original file line number	Diff line number	Diff line change
`@@ -65,6 +65,7 @@ protected void setupTask(SpotlessTask task) {`
`65`	`65`	`* Hence file extension based filtering is used in line with the org.eclipse.core.contenttype.contentTypes<`
`66`	`66`	`* defined by the CDT plugin.`
`67`	`67`	`*/`
	`68`	`+ noDefaultTarget();`
`68`	`69`	`target(CppDefaults.FILE_FILTER.toArray());`
`69`	`70`	`}`
`70`	`71`	`super.setupTask(task);`
Original file line number	Diff line number	Diff line change
`@@ -671,4 +671,8 @@ public SpotlessApply createIndependentApplyTask(String taskName) {`
`671`	`671`
`672`	`672`	`return applyTask;`
`673`	`673`	`}`
	`674`	`+`
	`675`	`+ protected void noDefaultTarget() {`
	`676`	`+ getProject().getLogger().warn("Spotless: no target set for " + formatName() + ", will be an error in the next release!");`
	`677`	`+ }`
`674`	`678`	`}`
Original file line number	Diff line number	Diff line change
`@@ -62,6 +62,7 @@ public void propertiesFile(Object... files) {`
`62`	`62`	`protected void setupTask(SpotlessTask task) {`
`63`	`63`	`// defaults to all markdown files`
`64`	`64`	`if (target == null) {`
	`65`	`+ noDefaultTarget();`
`65`	`66`	`target = parseTarget("*/.md");`
`66`	`67`	`}`
`67`	`68`	`super.setupTask(task);`
Original file line number	Diff line number	Diff line change
`@@ -57,6 +57,7 @@ private FormatterStep createStep() {`
`57`	`57`	`protected void setupTask(SpotlessTask task) {`
`58`	`58`	`if (target == null) {`
`59`	`59`	`target("*/.sql");`
	`60`	`+ noDefaultTarget();`
`60`	`61`	`}`
`61`	`62`	`super.setupTask(task);`
`62`	`63`	`}`
Original file line number	Diff line number	Diff line change
`@@ -170,6 +170,7 @@ private void fixParserToTypescript() {`
`170`	`170`	`protected void setupTask(SpotlessTask task) {`
`171`	`171`	`// defaults to all typescript files`
`172`	`172`	`if (target == null) {`
	`173`	`+ noDefaultTarget();`
`173`	`174`	`target = parseTarget("*/.ts");`
`174`	`175`	`}`
`175`	`176`	`super.setupTask(task);`