add support for missing generic steps (closes #207) #209

matthiasbalke · 2018-02-21T09:03:55Z

This PR adds support for missing generic steps within the maven-plugin.

All new steps are available in the new format configuration section, but also within java and scala.

Please review if anything is missing or should be changed.

add format section for custom formats

matthiasbalke · 2018-02-21T09:05:12Z

On Mac OS X I cannot reformat the project as I get testCompile errors. Maybe I have to reformat the code on Windows. I'll file an issue for this Mac OS X bug later.

nedtwigg · 2018-02-21T18:09:17Z

testCompile errors? Can you put stacktrace of gradlew spotlessApply? I do most of my spotless work on mac, haven't seen this issue.

nedtwigg

Thanks very much, this is great work!

This commit changes 28 files, which is quite a lot. In the future, it would be better to split this PR up into multiple commits. Intermediate commits make it easier to follow your train of thought, and easier to give feedback on alternate design choices. Ideally, I would have broken this up as follows:

One commit to create com.diffplug.spotless.maven.generic.Format
One commit per-formatter that is introduced

It's very easy to squash commits together later, but much harder to cut them up. Err on the side of showing too much of your work :) No need to start from scratch, we can work with this as-is, just for future reference.

There are some things that need to change before merging, I've left my comments inline.

nedtwigg · 2018-02-21T21:04:05Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Indent.java

+		System.out.println("withTabs: " + withTabs);
+		System.out.println("withSpaces: " + withSpaces);
+		System.out.println("spacesPerTab: " + spacesPerTab);
+


Debugging should be stripped out.

😅 obviously

nedtwigg · 2018-02-21T21:05:11Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Indent.java

+public class Indent implements FormatterStepFactory {
+
+	private static final int DEFAULT_NUM_SPACES_PER_TAB = 4;
+


This constant should either be defined in IndentStep, and accessed here as a public static int defaultNumSpacesPerTab(), or there shouldn't be a default at all. Otherwise the gradle and maven will eventually get out of sync. Mostly I think there shouldn't be a default at all, but if the gradle plugin already has one then good on you for matching behavior :)

fixed: moved default value into public static int defaultNumSpacesPerTab() and refactored maven and gradle plugin.

nedtwigg · 2018-02-21T21:09:15Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Indent.java

+
+		if (!withTabs && !withSpaces) {
+			return IndentStep.create(IndentStep.Type.SPACE, amountOfSpacesPerTab);
+		} else {


I wouldn't be forgiving about this. Easier for users to debug something that gives them a clear error message than silently guessing defaults that aren't obvious. One way to check that the user set exactly one to true is XOR:

if (withTabs ^ withSpaces) { createStep } else { error }

I thought this would match the maven convetion over configuration pattern. But I'm ok with not having defaults, if this is what spotless does for gradle.

But default includes for java, scala, etc. should be kept right?

IMO there's two good reasons for convention over configuration:

A) The default behavior is appropriate for 80%+ of use cases. e.g. we default to GIT_ATTRIBUTES for handling line endings
B) There are lots of arbitrary decisions to make, and we are providing a comprehensive solution. e.g. maven's layout of source directories, or google-java-format formatting all kinds of things with no configuration

The problem with indent is that the tabs vs spaces debate is alive and well - no 80+% default. And it's not comprehensive - there's only 2 decisions to make (tabs vs space, tab:space ratio) - so we're not really reducing the user's burden by making default decisions for the user.

Let's keep default includes - those are appropriate for 80%+ of use cases.

I agree to your explanation.

nedtwigg · 2018-02-21T21:10:51Z

plugin-maven/src/test/java/com/diffplug/spotless/maven/generic/IndentTest.java

+		mavenRunner().withArguments("spotless:apply").runNoError();
+		String actual = read("src/main/java/test.java");
+		assertThat(actual).isEqualTo(getTestResource(target));
+	}


assertFile("src/main/java/test.java").sameAsResource(target)

I used the test structure of the existing plugin-maven tests, so I wasn't aware of this. Should I refactor the existing tests too?

Ah, stuff slips through the cracks all the time, especially for a giant change like adding a whole new plugin. It'd be great to have the existing tests refactored. No obligation to do it though.

nedtwigg · 2018-02-21T21:11:28Z

plugin-maven/src/test/java/com/diffplug/spotless/maven/generic/LineEndingsTest.java

+		mavenRunner().withArguments("spotless:apply").runNoError();
+		String actual = read("src/main/java/test.java");
+		assertThat(actual).isEqualTo(getTestResource(target));
+	}


assertFile("src/main/java/test.java").sameAsResource(target)

nedtwigg · 2018-02-21T21:14:55Z

.gitattributes

@@ -1,3 +1,6 @@
 * text eol=lf
 *.png binary
 *.jar binary
+
+# for newline tests this file needs crlf
+testlib/src/main/resources/lineEndings/JavaCode-WINDOWS.test eol=crlf


Remove this, and all of the test resources. It's better to create newline tests with explicit string constants, e.g.:

String noTrailingNewline = "public class Java {}" String hasTrailingNewline = noEndingNewline + "\n"; setFile("path").toContent(noTrailingNewline); runTest(); assertFile("path").hasContent(hasTrailingNewline);

Most editors (github included) makes it hard to see trailing newlines and unix/windows differences. Best to just write them explicitly when you're writing a test for them.

I totaly agree.

fixed for newline and line endings tests

Did you mean all test resources I created, or only the ones testing newline formats?

nedtwigg · 2018-02-21T21:17:33Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Replace.java

+
+	@Override
+	public FormatterStep newFormatterStep(FormatterStepConfig config) {
+		if (name == null || search == null ||replacement == null) {


I bet search == null ||replacement is what the formatter is choking on, but there might be others. spotlessApply to the rescue!

nedtwigg · 2018-02-21T21:18:32Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/TrimTrailingWhitespace.java

+	@Override
+	public FormatterStep newFormatterStep(FormatterStepConfig config) {
+		return ReplaceRegexStep.create("trimTrailingWhitespace", "[ \t]+$", "");
+	}


Is this how trimTrailingWhitespace is implemented in Gradle? If so, it should be refactored into lib. Code duplication makes it hard to maintain later.

This must be fixed before merging. I accidently marked this as fixed!

nedtwigg · 2018-02-21T21:21:29Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Format.java

+	public void addReplaceRegex(ReplaceRegex replaceRegex) {
+		addStepFactory(replaceRegex);
+	}
+}


So, FormatterFactory.java has exactly one subclass, Format.java, and every other format (e.g. Java, Scala) will extend Format.java and not FormatterFactory.java, is that correct? I wonder if Format and FormatterFactory ought to be merged? Thoughts @lutovich?

Extending Format was quite a last-minute change. Haven't thought about merging those classes, but I agree that this should be considered. Which class should I delete (merge into the other)?

Keep them separate for now. Hopefully @lutovich will have a chance to take a look.

Am I right that Format is needed to keep all injectable generic steps in one class, which is reused for Java, Scala and AbstractSpotlessMojo?

Would it make sense to keep Format and FormatterFactory separate but move all #add* methods to FormatterFactory? Then Java and Scala can keep extending FormatterFactory.

The class Format aggregates all generic steps, which can be used to build custom formats, but also for <java>, <scala>, <kotlin> or <markdown> if you like. If using a formatter step like eclipse or googleFormat this won't make any sense most of the time. But formatting an markdown file might use some of the generic steps.

Moreover IMHO the maven-plugin should support the same feature set as the gradle-plugin does. And the gradle-plugin supports it this way.

So one could setup the formatting like this

<configuration>  <encoding>UTF-8</encoding>  <format> <inlcudes> <include>src/**/resources/**/*.properties</include> </includes> <endWithNewline /> <encoding>ISO 8859-1</encoding> </format>  <java> <endWithNewline /> <trimTrailingWhitespace /> </java> </configuration>

nedtwigg · 2018-02-21T21:23:32Z

plugin-maven/src/test/java/com/diffplug/spotless/maven/generic/EndWithNewlineTest.java

+		mavenRunner().withArguments("spotless:apply").runNoError();
+		String actual = read("src/main/java/test.java");
+		assertThat(actual).isEqualTo(getTestResource("newline/HasNewline.test"));
+	}


assertFile("src/main/java/test.java").sameAsResource(target)

matthiasbalke · 2018-02-22T09:56:15Z

Thanks for your feedback. I'll fix these parts tonight.

Sorry about squashing. I thought it's easier for you to review the final version. I commited the changes nearly as you expected it ;)

lutovich

I'm a bit confused by the new <format> section. It is supposed to be like a container with default steps for defined formatters (java, scala) but also defines it's own includes and excludes.

lutovich · 2018-02-22T16:01:22Z

plugin-maven/README.md

+
+```xml
+<configuration>
+  <format>


What do you think about not having <format> element? Configuration could then look like:

<configuration> <endWithNewline /> <trimTrailingWhitespace /> <replace> <name>Say Hello to Mars</name> <search>World</search> <replacement>Mars</replacement> </replace> <java> <eclipse>...</eclipse> </java> <scala>...</scala> </configuration>

?

I think the purpose of the <format> element is to do specific formatting on specific kinds of files that aren't java or scala. e.g.:

spotless/spotlessSelf.gradle

Lines 54 to 59 in 4d28488

format 'misc', {

target '**/*.md', '**/*.gitignore'

indentWithSpaces(2)

trimTrailingWhitespace()

endWithNewline()

}

Aha, got it. This explains a lot :)
Please dismiss all my previous comments about <format>.

Maybe then we need an ability to configure multiple formats?

The idea of the new format section is to provide the ability to define custom formats for e.g. property and sql files. Currently only one format section is allowed, but the gradle plugin allows unlimited format sections. This way you can define a format per source set. This comes in quite handy on larger projects.

I'm planing to extend the maven-plugin to provide unlimited format sections too.

lutovich · 2018-02-22T16:02:48Z

plugin-maven/README.md

+    <!-- Specify whether to use tabs or spaces for indentation -->
+    <indent>
+      <!-- Specify either withSpaces or withTabs. Defaults to withSpaces -->
+      <withSpaces>true</withSpaces>


Maybe <spaces> instead of <withSpaces> and <tabs> instead of <withTabs>?

I have thought about that too. The idea was to provide better readability (indent > withSpaces), but I guess thats not common for maven plugins. It's more of the Groovy DSL style.

I'm fine with spaces and tabs.

lutovich · 2018-02-22T16:16:06Z

plugin-maven/README.md

+    <includes>
+      <!-- include all property files in "resource" folders under "src" -->
+      <include>src/**/resources/**/*.properties</include>
+    </includes>


Does this section define default includes?
I think elements from <format> should be applied only to files defined by <java> and <scala>. So maybe we do not even need <includes> and <excludes> in <format>?

No the <format> section does not define default includes, as you don't know beforehand what its used for see above (properties, sql ,..).

lutovich · 2018-02-22T16:20:39Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java

@@ -71,6 +71,9 @@
 	@Parameter
 	private LicenseHeader licenseHeader;

+	@Parameter
+	private Format format;


Existence of both LicenseHeader and Format field permits the following configuration:

<configuration> <licenseHeader>...</licenseHeader> <format> <licenseHeader>...</licenseHeader> </format> </configuration>

which seems invalid because <format> is like a global-default.

In the gradle plugin, <format> is not a global default. It's a way for using manual steps like replace and indent to do specific formatting on specific files which aren't handled well by language-specific stuff like java and scala.

I don't understand what you mean. It's 100% the same as with global and <java>, isn't it? You could use a different licenseHeader per source set. Even if this doesn't sound like the most common use case.

Please never mind all my comments about <format>. I did not understand what it is.

lutovich · 2018-02-22T16:26:01Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Replace.java

+
+public class Replace implements FormatterStepFactory {
+
+	@Parameter


@Parameter(required = true) can probably be used for all fields in this class. Then null checks will not be required

I used the annotation parameter in my first iteration, but that leads to validation messages which are hard to understand for users:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:1.11.0-SNAPSHOT:check (default-cli) on project demo: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:1.11.0-SNAPSHOT:check failed: name -> [Help 1]

Where as a custom handling is shown like that:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:1.11.0-SNAPSHOT:check (default-cli) on project demo: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:1.11.0-SNAPSHOT:check failed: Must specify 'name', 'search' and 'replacement'. -> [Help 1]

Thats why I prefer custom validation for required parameters and would like to keep it.

lutovich · 2018-02-22T16:26:33Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/ReplaceRegex.java

+
+public class ReplaceRegex implements FormatterStepFactory {
+
+	@Parameter


@Parameter(required = true) can probably be used for all fields in this class

lutovich · 2018-02-22T16:30:04Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Format.java

+	public void addReplaceRegex(ReplaceRegex replaceRegex) {
+		addStepFactory(replaceRegex);
+	}
+}


Am I right that Format is needed to keep all injectable generic steps in one class, which is reused for Java, Scala and AbstractSpotlessMojo?

matthiasbalke · 2018-02-22T19:14:37Z

@nedtwigg, @lutovich I fixed your review findings. Please review again.

nedtwigg · 2018-02-22T19:30:10Z

Looks great! There's still resources/trailinWhitespace/*, and resources/replace/*. I think both of these would be easier to test with string literals rather than test resources. Trailing whitespace is hard to see, and the replace files only make sense if you know what is being replaced with what.

setFile("file").toContent("Hello world");
replace("world", "bob")
assertFile("file").hasContent("Hello bob");

is easier to read and maintain. The test resources are best for the comprehensive steps (e.g. google-java-format) where it's hard to make a good test case with a simple string literal.

matthiasbalke · 2018-02-22T19:46:19Z

I fixed it, but cannot push currently. I get authentication failures but don't know why. I'll push it tomorrow.

matthiasbalke · 2018-02-23T06:36:25Z

ready for review

nedtwigg

Great!!

nedtwigg · 2018-02-23T07:34:43Z

I'm gonna let @lutovich push the merge button on this. There were some unresolved issues with allowing multiple <format> blocks, but that might be best handled as a new PR.

matthiasbalke · 2018-02-23T07:42:14Z

I just realized, that I didn't add this feature to Changes.md. Are you goint to do that before merging, or should I update it by myself?

Also we talked about refcatoring the implementation of TrimTrailingWhitespace into lib. I marked this accidently fixed. I'll have to fix that before merging.

@nedtwigg I agree whith starting a new PR for multiple formats. I'll file a new issue for that.

@nedtwigg, @lutovich Also do you want to squash merge this as a whole, because I did some commits to fix the review findings?

nedtwigg · 2018-02-23T15:28:54Z

Don't worry about squashing - we can press the "squash" button when we merge on GitHub. This particular one seems easier to read after squashing, so that's probably what we'll end up doing.

nedtwigg · 2018-02-23T15:29:25Z

Re: CHANGES.md - please update yourself

lutovich

Looks good! Multiple formats can be a separate PR. I'll merge when changelog is updated.

matthiasbalke · 2018-02-26T06:54:36Z

@lutovich ready for squash merge 😄

add support for missing generic steps (closes diffplug#207)

0a3d542

add format section for custom formats

nedtwigg requested changes Feb 21, 2018

View reviewed changes

lutovich reviewed Feb 22, 2018

View reviewed changes

matthiasbalke force-pushed the maven-formatter branch 2 times, most recently from 048e15a to f20133b Compare February 22, 2018 19:03

matthiasbalke added 3 commits February 22, 2018 20:12

reformated code

98bfe1e

fixed review findings

0ab7ca0

use assertFile instead of assertThat.isEqualTo for all tests

23a1d1b

matthiasbalke force-pushed the maven-formatter branch from f20133b to 23a1d1b Compare February 22, 2018 19:12

fixed review findings

2256c73

nedtwigg approved these changes Feb 23, 2018

View reviewed changes

matthiasbalke added 2 commits February 23, 2018 09:08

Merge remote-tracking branch 'upstream/master' into maven-formatter

c53962a

fixed code duplication

b2eee6f

lutovich approved these changes Feb 23, 2018

View reviewed changes

lutovich mentioned this pull request Feb 23, 2018

Improved support for multi-module maven projects #210

Merged

updated changelog

9047612

lutovich merged commit 2dd2157 into diffplug:master Feb 26, 2018

matthiasbalke deleted the maven-formatter branch February 26, 2018 10:39

		public class Indent implements FormatterStepFactory {

		private static final int DEFAULT_NUM_SPACES_PER_TAB = 4;

	format 'misc', {
	target '*/.md', '*/.gitignore'
	indentWithSpaces(2)
	trimTrailingWhitespace()
	endWithNewline()
	}


		public class Replace implements FormatterStepFactory {

		@Parameter


		public class ReplaceRegex implements FormatterStepFactory {

		@Parameter

add support for missing generic steps (closes #207) #209

add support for missing generic steps (closes #207) #209

Conversation

matthiasbalke commented Feb 21, 2018

matthiasbalke commented Feb 21, 2018

nedtwigg commented Feb 21, 2018

nedtwigg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke commented Feb 22, 2018

lutovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

lutovich Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbalke commented Feb 22, 2018

nedtwigg commented Feb 22, 2018

matthiasbalke commented Feb 22, 2018

matthiasbalke commented Feb 23, 2018

nedtwigg left a comment

Choose a reason for hiding this comment

nedtwigg commented Feb 23, 2018

matthiasbalke commented Feb 23, 2018 • edited Loading

nedtwigg commented Feb 23, 2018

nedtwigg commented Feb 23, 2018

lutovich left a comment

Choose a reason for hiding this comment

matthiasbalke commented Feb 26, 2018 • edited Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

lutovich Feb 22, 2018 •

edited

Loading

matthiasbalke Feb 22, 2018 •

edited

Loading

matthiasbalke commented Feb 23, 2018 •

edited

Loading

matthiasbalke commented Feb 26, 2018 •

edited

Loading