Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Feb 26, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ function extra(className) { return '| [`' + className + '`](lib-extra/src/main/j
output = [
'| Feature / FormatterStep | [plugin-gradle](plugin-gradle/README.md) | [plugin-maven](plugin-maven/README.md) | [(Your build tool here)](CONTRIBUTING.md#how-to-add-a-new-plugin-for-a-build-system) |',
'| --------------------------------------------- | ------------- | ------------ | --------|',
lib('generic.EndWithNewlineStep') +'{{yes}} | {{no}} | {{no}} |',
lib('generic.IndentStep') +'{{yes}} | {{no}} | {{no}} |',
lib('generic.EndWithNewlineStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('generic.IndentStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('generic.LicenseHeaderStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('generic.ReplaceRegexStep') +'{{yes}} | {{no}} | {{no}} |',
lib('generic.ReplaceStep') +'{{yes}} | {{no}} | {{no}} |',
lib('generic.TrimTrailingWhitespaceStep') +'{{yes}} | {{no}} | {{no}} |',
lib('generic.ReplaceRegexStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('generic.ReplaceStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('generic.TrimTrailingWhitespaceStep') +'{{yes}} | {{yes}} | {{no}} |',
extra('groovy.GrEclipseFormatterStep') +'{{yes}} | {{no}} | {{no}} |',
lib('java.GoogleJavaFormatStep') +'{{yes}} | {{yes}} | {{no}} |',
lib('java.ImportOrderStep') +'{{yes}} | {{yes}} | {{no}} |',
Expand All @@ -55,12 +55,12 @@ lib('sql.DBeaverSQLFormatterStep') +'{{yes}} | {{no}}
-->
| Feature / FormatterStep | [plugin-gradle](plugin-gradle/README.md) | [plugin-maven](plugin-maven/README.md) | [(Your build tool here)](CONTRIBUTING.md#how-to-add-a-new-plugin-for-a-build-system) |
| --------------------------------------------- | ------------- | ------------ | --------|
| [`generic.EndWithNewlineStep`](lib/src/main/java/com/diffplug/spotless/generic/EndWithNewlineStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`generic.IndentStep`](lib/src/main/java/com/diffplug/spotless/generic/IndentStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`generic.EndWithNewlineStep`](lib/src/main/java/com/diffplug/spotless/generic/EndWithNewlineStep.java) | :+1: | :+1: | :white_large_square: |
| [`generic.IndentStep`](lib/src/main/java/com/diffplug/spotless/generic/IndentStep.java) | :+1: | :+1: | :white_large_square: |
| [`generic.LicenseHeaderStep`](lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java) | :+1: | :+1: | :white_large_square: |
| [`generic.ReplaceRegexStep`](lib/src/main/java/com/diffplug/spotless/generic/ReplaceRegexStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`generic.ReplaceStep`](lib/src/main/java/com/diffplug/spotless/generic/ReplaceStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`generic.TrimTrailingWhitespaceStep`](lib/src/main/java/com/diffplug/spotless/generic/TrimTrailingWhitespaceStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`generic.ReplaceRegexStep`](lib/src/main/java/com/diffplug/spotless/generic/ReplaceRegexStep.java) | :+1: | :+1: | :white_large_square: |
| [`generic.ReplaceStep`](lib/src/main/java/com/diffplug/spotless/generic/ReplaceStep.java) | :+1: | :+1: | :white_large_square: |
| [`generic.TrimTrailingWhitespaceStep`](lib/src/main/java/com/diffplug/spotless/generic/TrimTrailingWhitespaceStep.java) | :+1: | :+1: | :white_large_square: |
| [`groovy.GrEclipseFormatterStep`](lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java) | :+1: | :white_large_square: | :white_large_square: |
| [`java.GoogleJavaFormatStep`](lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java) | :+1: | :+1: | :white_large_square: |
| [`java.ImportOrderStep`](lib/src/main/java/com/diffplug/spotless/java/ImportOrderStep.java) | :+1: | :+1: | :white_large_square: |
Expand Down
12 changes: 12 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/generic/IndentStep.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@

/** Simple step which checks for consistent indentation characters. */
public final class IndentStep {

private static final int DEFAULT_NUM_SPACES_PER_TAB = 4;

// prevent direct instantiation
private IndentStep() {}

Expand All @@ -33,6 +36,11 @@ private <T> T tabSpace(T tab, T space) {
return this == TAB ? tab : space;
}

/** Creates a step which will indent with the given type of whitespace, converting between tabs and spaces at the default ratio. */
public FormatterStep create() {
return IndentStep.create(this, defaultNumSpacesPerTab());
}

/** Synonym for {@link IndentStep#create(Type, int)}. */
public FormatterStep create(int numSpacesPerTab) {
return IndentStep.create(this, numSpacesPerTab);
Expand Down Expand Up @@ -128,4 +136,8 @@ String format(String raw) {
private static boolean isSpaceOrTab(char c) {
return c == ' ' || c == '\t';
}

public static int defaultNumSpacesPerTab() {
return DEFAULT_NUM_SPACES_PER_TAB;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ public void indentWithSpaces(int numSpacesPerTab) {

/** Ensures that the files are indented using spaces. */
public void indentWithSpaces() {
indentWithSpaces(4);
addStep(IndentStep.Type.SPACE.create());
}

/** Ensures that the files are indented using tabs. */
Expand All @@ -342,7 +342,7 @@ public void indentWithTabs(int tabToSpaces) {

/** Ensures that the files are indented using tabs. */
public void indentWithTabs() {
indentWithTabs(4);
addStep(IndentStep.Type.TAB.create());
}

abstract class LicenseHeaderConfig {
Expand Down
54 changes: 54 additions & 0 deletions plugin-maven/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,60 @@ By default, all files matching `src/main/java/**/*.java` and `src/test/java/**/*
</configuration>
```

<a name="format"></a>

## Applying to custom sources

By default, no Ant-Style include patterns are defined. Each element under `<format>` is a step, and they will be applied in the order specified. Every step is optional, and they will be applied in the order specified.

```xml
<configuration>
<format>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<licenseHeader>
<!-- Specify either content or file, but not both -->
<content>/* Licensed under Apache-2.0 */</content>
<file>${basedir}/license-header</file>
<!-- conent until first occurrence of the delimiter regex will be interpreted as header section -->
<delimiter>#</delimiter>
</licenseHeader>

<!-- Files must end with a newline -->
<endWithNewline />

<!-- Specify whether to use tabs or spaces for indentation -->
<indent>
<!-- Specify either spaces or tabs -->
<spaces>true</spaces>
<tabs>true</tabs>
<!-- Specify how many spaces are used to convert one tab and vice versa. Defaults to 4 -->
<spacesPerTab>4</spacesPerTab>
</indent>

<!-- Trim trailing whitespaces -->
<trimTrailingWhitespace />

<!-- Specify replacements using search and replace -->
<replace>
<name>Say Hello to Mars</name>
<search>World</search>
<replacement>Mars</replacement>
</replace>

<!-- Specify replacements using regex match and replace -->
<replaceRegex>
<name>Say Hello to Mars from Regex</name>
<searchRegex>(Hello) W[a-z]{3}d</searchRegex>
<replacement>$1 Mars</replacement>
</replaceRegex>
</format>
</configuration>
```

<a name="invisible"></a>

## Line endings and encodings (invisible stuff)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

import org.apache.maven.plugin.AbstractMojo;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.Component;
import org.apache.maven.plugins.annotations.Parameter;
import org.codehaus.plexus.util.FileUtils;
Expand All @@ -38,6 +37,7 @@
import com.diffplug.spotless.Formatter;
import com.diffplug.spotless.LineEnding;
import com.diffplug.spotless.Provisioner;
import com.diffplug.spotless.maven.generic.Format;
import com.diffplug.spotless.maven.generic.LicenseHeader;
import com.diffplug.spotless.maven.java.Java;
import com.diffplug.spotless.maven.scala.Scala;
Expand Down Expand Up @@ -71,6 +71,9 @@ public abstract class AbstractSpotlessMojo extends AbstractMojo {
@Parameter
private LicenseHeader licenseHeader;

@Parameter
private Format format;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@matthiasbalke matthiasbalke Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lutovich lutovich Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@Parameter
private Java java;

Expand All @@ -80,7 +83,7 @@ public abstract class AbstractSpotlessMojo extends AbstractMojo {
protected abstract void process(List<File> files, Formatter formatter) throws MojoExecutionException;

@Override
public final void execute() throws MojoExecutionException, MojoFailureException {
public final void execute() throws MojoExecutionException {
List<FormatterFactory> formatterFactories = getFormatterFactories();

for (FormatterFactory formatterFactory : formatterFactories) {
Expand Down Expand Up @@ -127,7 +130,7 @@ private FormatterConfig getFormatterConfig() {
}

private List<FormatterFactory> getFormatterFactories() {
return Stream.of(java, scala)
return Stream.of(format, java, scala)
.filter(Objects::nonNull)
.collect(toList());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.generic;

import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.generic.EndWithNewlineStep;
import com.diffplug.spotless.maven.FormatterStepConfig;
import com.diffplug.spotless.maven.FormatterStepFactory;

public class EndWithNewline implements FormatterStepFactory {

@Override
public FormatterStep newFormatterStep(FormatterStepConfig config) {
return EndWithNewlineStep.create();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.generic;

import java.util.Collections;
import java.util.Set;

import com.diffplug.spotless.maven.FormatterFactory;

public class Format extends FormatterFactory {

@Override
public Set<String> defaultIncludes() {
return Collections.emptySet();
}

@Override
public String licenseHeaderDelimiter() {
// do not specify a default delimiter
return null;
}

public void addEndWithNewline(EndWithNewline endWithNewline) {
addStepFactory(endWithNewline);
}

public void addIndent(Indent indent) {
addStepFactory(indent);
}

public void addTrimTrailingWhitespace(TrimTrailingWhitespace trimTrailingWhitespace) {
addStepFactory(trimTrailingWhitespace);
}

public void addReplace(Replace replace) {
addStepFactory(replace);
}

public void addReplaceRegex(ReplaceRegex replaceRegex) {
addStepFactory(replaceRegex);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@matthiasbalke matthiasbalke Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
  <!-- global defaults -->
  <encoding>UTF-8</encoding>

  <!-- custom format for properties -->
  <format>
    <inlcudes>
      <include>src/**/resources/**/*.properties</include>
    </includes>
    <endWithNewline />
    <encoding>ISO 8859-1</encoding>
  </format>

  <!-- custom format for java files -->
  <java>
    <endWithNewline />
    <trimTrailingWhitespace />
  </java>
</configuration>

Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.generic;

import org.apache.maven.plugins.annotations.Parameter;

import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.generic.IndentStep;
import com.diffplug.spotless.maven.FormatterStepConfig;
import com.diffplug.spotless.maven.FormatterStepFactory;

public class Indent implements FormatterStepFactory {

@Parameter
private boolean tabs;

@Parameter
private boolean spaces;

@Parameter
private Integer spacesPerTab;

@Override
public FormatterStep newFormatterStep(FormatterStepConfig config) {

if (spacesPerTab == null) {
spacesPerTab = IndentStep.defaultNumSpacesPerTab();
}

if (spaces ^ tabs) {
if (spaces) {
return IndentStep.create(IndentStep.Type.SPACE, spacesPerTab);
} else {
return IndentStep.create(IndentStep.Type.TAB, spacesPerTab);
}
} else {
throw new IllegalArgumentException("Must specify exactly one of 'spaces' or 'tabs'.");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.maven.generic;

import org.apache.maven.plugins.annotations.Parameter;

import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.generic.ReplaceStep;
import com.diffplug.spotless.maven.FormatterStepConfig;
import com.diffplug.spotless.maven.FormatterStepFactory;

public class Replace implements FormatterStepFactory {

@Parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@matthiasbalke matthiasbalke Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private String name;

@Parameter
private String search;

@Parameter
private String replacement;

@Override
public FormatterStep newFormatterStep(FormatterStepConfig config) {
if (name == null || search == null || replacement == null) {
throw new IllegalArgumentException("Must specify 'name', 'search' and 'replacement'.");
}

return ReplaceStep.create(name, search, replacement);
}
}
Loading