-
Notifications
You must be signed in to change notification settings - Fork 459
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
Initial version of Spotless Maven plugin #188
Changes from 1 commit
5ff5581
9956473
c219ef1
a5a6cc9
1d63aa2
af677ab
b059d53
c0a2fa6
a431b71
3639269
240f87c
8570064
75f0204
40da7f5
3696310
49c2753
35da59a
fd38755
3864178
d54b183
ddbf0eb
4754a8f
88bcd19
c84becf
012b118
0c01e64
c43ed45
66bb67e
31d5357
4b032eb
4cf839a
622d2df
e759a0b
0185af6
0dba10c
9bb5bbc
3189d7d
6568440
21f8d00
8ebdbfc
24f7096
5e9fce6
d95f76d
4f2952c
1d82ba9
4b34991
7db3759
d8258bd
a8de57d
3ca2bf7
807291e
48cb4c9
08f90d2
5ccbd09
7df6191
a676bc4
d4cddc0
b9a4fa4
bafc30c
9d3ea05
e438881
f8bf47d
d2170fe
da36b36
07b6d68
243035e
ea6ebb5
6d660c6
aef8b32
21f163b
e7cea18
da77c39
34ff0e9
7de4e1a
8e77cf7
fcebff7
53d8013
d52a448
079e9a2
3fdc785
95c7fc9
bd8a3b9
d215905
2345cbd
0f9ff8e
9cee266
15e65a0
740317c
a154f59
49987d7
81477a6
a934a73
4162569
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* 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.gradle.spotless; | ||
|
||
import java.io.IOException; | ||
|
||
import org.assertj.core.api.Assertions; | ||
import org.junit.Ignore; | ||
import org.junit.Test; | ||
|
||
/** | ||
* This test is ignored because it doesn't work. | ||
* To make it work, we'll need something like the following: | ||
* | ||
* build.gradle | ||
* task publishTestToLocalRepo() | ||
* test.dependsOn(publishTestToLocalRepo) | ||
* | ||
* Then in MavenIntegratonTest.POM_HEADER, set <pluginRepositories> appropriately | ||
* | ||
* Would also be good if we had mvnw setup, so that | ||
* the test harness used mvnw to control the verison | ||
* of maven that was used. | ||
*/ | ||
@Ignore | ||
public class EclipseFormatStepTest extends MavenIntegrationTest { | ||
@Test | ||
public void testEclipse() throws IOException, InterruptedException { | ||
// write the pom | ||
writePomJavaSteps( | ||
"<eclipse>", | ||
" <file>${basedir}/eclipse-fmt.xml</file>", | ||
" <version>4.7.1</version>", | ||
"</eclipse>"); | ||
write("src/main/java/test.java", getTestResource("java/eclipse/format/JavaCodeUnformatted.test")); | ||
mavenRunner().withArguments("spotless:apply").runNoError(); | ||
|
||
String actual = read("src/main/java/test.java"); | ||
Assertions.assertThat(actual).isEqualTo(getTestResource("java/eclipse/format/JavaCodeFormatted.test")); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
/* | ||
* 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.gradle.spotless; | ||
|
||
import java.io.IOException; | ||
import java.util.Arrays; | ||
import java.util.stream.Collectors; | ||
|
||
import org.junit.Before; | ||
|
||
import com.diffplug.spotless.ResourceHarness; | ||
|
||
public class MavenIntegrationTest extends ResourceHarness { | ||
/** | ||
* Each test gets its own temp folder, and we create a maven | ||
* build there and run it. | ||
* | ||
* Because those test folders don't have a .gitattributes file, | ||
* git on windows will default to \r\n. So now if you read a | ||
* test file from the spotless test resources, and compare it | ||
* to a build result, the line endings won't match. | ||
* | ||
* By sticking this .gitattributes file into the test directory, | ||
* we ensure that the default Spotless line endings policy of | ||
* GIT_ATTRIBUTES will use \n, so that tests match the test | ||
* resources on win and linux. | ||
*/ | ||
@Before | ||
public void gitAttributes() throws IOException { | ||
write(".gitattributes", "* text eol=lf"); | ||
} | ||
|
||
private static final String POM_HEADER = "" + | ||
"<project>\n" + | ||
" <modelVersion>4.0.0</modelVersion>\n" + | ||
" <repositories>\n" + | ||
" <repository>\n" + | ||
" <id>central</id>\n" + | ||
" <name>Central Repository</name>\n" + | ||
" <url>http://repo.maven.apache.org/maven2</url>\n" + | ||
" <layout>default</layout>\n" + | ||
" <snapshots>\n" + | ||
" <enabled>false</enabled>\n" + | ||
" </snapshots>\n" + | ||
" </repository>\n" + | ||
" </repositories>\n" + | ||
" <pluginRepositories>\n" + // TODO: setup test so that the plugin gets compiled first, and put into this repository | ||
" <pluginRepository>\n" + | ||
" <id>central</id>\n" + | ||
" <name>Central Repository</name>\n" + | ||
" <url>http://repo.maven.apache.org/maven2</url>\n" + | ||
" <layout>default</layout>\n" + | ||
" <snapshots>\n" + | ||
" <enabled>false</enabled>\n" + | ||
" </snapshots>\n" + | ||
" <releases>\n" + | ||
" <updatePolicy>never</updatePolicy>\n" + | ||
" </releases>\n" + | ||
" </pluginRepository>\n" + | ||
" </pluginRepositories>\n" + | ||
" <build>\n" + | ||
" <plugins>\n" + | ||
" <plugin>\n" + | ||
" <groupId>com.diffplug.spotless</groupId>\n" + | ||
" <artifactId>spotless-maven-plugin</artifactId>\n" + | ||
" <version>+</version>\n" + | ||
" <configuration>\n"; | ||
|
||
private static final String POM_FOOTER = "" + | ||
" </configuration>\n" + | ||
" </plugin>\n" + | ||
" </plugins>\n" + | ||
" </build>\n" + | ||
"</project>\n"; | ||
|
||
protected void writePomJavaSteps(String... steps) throws IOException { | ||
write("pom.xml", | ||
POM_HEADER, | ||
"<java><steps>", | ||
Arrays.stream(steps).collect(Collectors.joining("\n")), | ||
"</steps></java>", | ||
POM_FOOTER); | ||
} | ||
|
||
protected MavenRunner mavenRunner() throws IOException { | ||
return MavenRunner.create() | ||
.withProjectDir(rootFolder()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
/* | ||
* 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.gradle.spotless; | ||
|
||
import java.io.ByteArrayOutputStream; | ||
import java.io.File; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.nio.charset.Charset; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Locale; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
import org.assertj.core.api.Assertions; | ||
|
||
import com.diffplug.common.base.Throwables; | ||
|
||
/** | ||
* Harness for running a maven build, same idea as the | ||
* [GradleRunner from the gradle testkit](https://docs.gradle.org/current/javadoc/org/gradle/testkit/runner/GradleRunner.html). | ||
*/ | ||
public class MavenRunner { | ||
public static MavenRunner create() { | ||
return new MavenRunner(); | ||
} | ||
|
||
private MavenRunner() {} | ||
|
||
private File projectDir; | ||
private String[] args; | ||
|
||
public MavenRunner withProjectDir(File projectDir) { | ||
this.projectDir = Objects.requireNonNull(projectDir); | ||
return this; | ||
} | ||
|
||
public MavenRunner withArguments(String... args) { | ||
this.args = Objects.requireNonNull(args); | ||
return this; | ||
} | ||
|
||
private Result run() throws IOException, InterruptedException { | ||
Objects.requireNonNull(projectDir, "Need to call withProjectDir() first"); | ||
Objects.requireNonNull(args, "Need to call withArguments() first"); | ||
// run maven with the given args in the given directory | ||
List<String> cmds = getPlatformCmds("mvn " + Arrays.stream(args).collect(Collectors.joining(" "))); | ||
ProcessBuilder builder = new ProcessBuilder(cmds); | ||
builder.directory(projectDir); | ||
Process process = builder.start(); | ||
// slurp and return the stdout, stderr, and exitValue | ||
Slurper output = new Slurper(process.getInputStream()); | ||
Slurper error = new Slurper(process.getErrorStream()); | ||
int exitValue = process.waitFor(); | ||
output.join(); | ||
error.join(); | ||
return new Result(exitValue, output.result(), error.result()); | ||
} | ||
|
||
/** Runs the command and asserts that nothing was printed to stderr. */ | ||
public Result runNoError() throws IOException, InterruptedException { | ||
Result result = run(); | ||
Assertions.assertThat(result.error()).isEmpty(); | ||
return result; | ||
} | ||
|
||
/** Runs the command and asserts that something was printed to stderr. */ | ||
public Result runHasError() throws IOException, InterruptedException { | ||
Result result = run(); | ||
Assertions.assertThat(result.error()).isNotEmpty(); | ||
return result; | ||
} | ||
|
||
public static class Result { | ||
private final int exitValue; | ||
private final String output; | ||
private final String error; | ||
|
||
public Result(int exitValue, String output, String error) { | ||
super(); | ||
this.exitValue = exitValue; | ||
this.output = Objects.requireNonNull(output); | ||
this.error = Objects.requireNonNull(error); | ||
} | ||
|
||
public int exitValue() { | ||
return exitValue; | ||
} | ||
|
||
public String output() { | ||
return output; | ||
} | ||
|
||
public String error() { | ||
return error; | ||
} | ||
} | ||
|
||
/** Prepends any arguments necessary to run a console command. */ | ||
private static List<String> getPlatformCmds(String cmd) { | ||
if (isWin()) { | ||
return Arrays.asList("cmd", "/c", cmd); | ||
} else { | ||
return Arrays.asList("/bin/sh", "-c", cmd); | ||
} | ||
} | ||
|
||
private static boolean isWin() { | ||
String os_name = System.getProperty("os.name").toLowerCase(Locale.getDefault()); | ||
return os_name.contains("win"); | ||
} | ||
|
||
private static class Slurper extends Thread { | ||
private final InputStream input; | ||
private volatile String result; | ||
|
||
Slurper(InputStream input) { | ||
this.input = Objects.requireNonNull(input); | ||
start(); | ||
} | ||
|
||
@Override | ||
public void run() { | ||
try { | ||
ByteArrayOutputStream output = new ByteArrayOutputStream(); | ||
byte[] buffer = new byte[1024]; | ||
int numRead; | ||
while ((numRead = input.read(buffer)) != -1) { | ||
output.write(buffer, 0, numRead); | ||
} | ||
result = output.toString(Charset.defaultCharset().name()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nedtwigg Minor nit: I seem to remember that we have access to Guava in some way through a re-packaged version that Diffplug distributes on Maven Central and JCenter. If we do have access to Guava in this way, then I think we should consider using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, implemented in next commit. For the rest of this review, I'd like to focus on bashing out the high-level design. We can fix performance nits and duplicate code after we have something working :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger! :) |
||
} catch (Exception e) { | ||
result = Throwables.getStackTraceAsString(e); | ||
} | ||
} | ||
|
||
public String result() { | ||
return result; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
public class Java { | ||
public static void main(String[] args) { | ||
System.out.println("hello"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
public class Java { | ||
public static void main(String[] args) { | ||
System.out.println("hello"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nedtwigg If we expect
MavenRunner
to be instantiated many times, I think we should consider using a sharedExecutorService
, like anExecutors.newFixedThreadPool(Runtime.getRuntime().getAvailableProcessors() - 1)
. Otherwise, this whole commit looks pretty reasonable to me. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I'm proposing we use an
ExecutorService
instead of usingThread
s directly through theSlurper
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger. There will only be 10's of integration tests, so I don't think there's much of a performance hit for starting 2x threads per.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that sounds reasonable to me.