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.

Sign up for GitHub

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

Fix issue #31 #45

Closed
wants to merge 16 commits into from
Closed

Fix issue #31 #45

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7e1fcad
Initial commit - does not compile!
jbduncan Oct 29, 2016
6061363
Intermediate commit - compiles but SelfTest fails!
jbduncan Oct 30, 2016
c25ac3c
3rd snapshot commit
jbduncan Oct 31, 2016
0faae70
Merged master, fixed various conflicts in the test infrastructure.
nedtwigg Oct 31, 2016
2c45080
`gradlew spotlessApply` works again.
nedtwigg Oct 31, 2016
0e5deb4
Unused import cleanup.
nedtwigg Oct 31, 2016
cec7db3
Even if there are no steps, spotless still enforces line endings.
nedtwigg Oct 31, 2016
7b6aea7
First cut at ForwardingEquality.
nedtwigg Nov 1, 2016
6113458
Morphed ForwardingEquality into LazyForwardingEquality.
nedtwigg Nov 1, 2016
452fbd9
Used LazyForwardingEquality to implement FormatterStep.Strict, which …
nedtwigg Nov 1, 2016
c1357f5
The new FormatterStep.create() and createLazy() utilized anonymous cl…
nedtwigg Nov 1, 2016
51f0531
Rewrote LicenseHeaderStep in terms of FormatterStep.Strict.
nedtwigg Nov 1, 2016
eefdc50
FormatExtension now has bumpThisNumberIfACustomRuleChanges(), so that…
nedtwigg Nov 1, 2016
40c00d0
Removed FormatterStep's deprecated methods and moved them to the test…
nedtwigg Nov 1, 2016
4262b2b
Attempt at excluding project build and cache dirs - doesn't pass tests
jbduncan Nov 1, 2016
fb613bc
LazyForwardingEquality now implements hashCode() and equals() for fre…
nedtwigg Nov 1, 2016
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
2 changes: 1 addition & 1 deletion src/main/java/com/diffplug/gradle/spotless/FormatExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ public void licenseHeaderFile(Object licenseHeaderFile, String delimiter) {
/** Sets up a FormatTask according to the values in this extension. */
protected void setupTask(FormatTask task) throws Exception {
task.paddedCell = paddedCell;
task.lineEndingsPolicy = getLineEndingPolicy();
task.lineEndingPolicy = getLineEndingPolicy();
Copy link
Member

Choose a reason for hiding this comment

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

I don't care which name we use, but this is a public member of a public class documented in the README, so changing it breaks API compat for everyday buildscripts. These changes are likely going to require a major version bump anyway, but I'd rather not churn the lineEndingsPolicy name.

aside: I've enjoyed learning about a lot of Stream methods I didn't know about before, lots of good cleanup in various parts 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm glad you're enjoying learning more about Stream methods. :D

task.encoding = getEncoding();
task.target = target;
task.steps = steps;
Expand Down
68 changes: 49 additions & 19 deletions src/main/java/com/diffplug/gradle/spotless/FormatTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,36 @@
import java.util.List;
import java.util.Locale;

import com.diffplug.common.base.Errors;
import org.gradle.api.DefaultTask;
import org.gradle.api.GradleException;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.InputFiles;
import org.gradle.api.tasks.OutputFiles;
import org.gradle.api.tasks.SkipWhenEmpty;
import org.gradle.api.tasks.TaskAction;
import org.gradle.api.tasks.incremental.IncrementalTaskInputs;

public class FormatTask extends DefaultTask {
// set by SpotlessExtension, but possibly overridden by FormatExtension
@Input
public Charset encoding = StandardCharsets.UTF_8;
public LineEnding.Policy lineEndingsPolicy = LineEnding.UNIX_POLICY;
@Input
public LineEnding.Policy lineEndingPolicy = LineEnding.UNIX_POLICY;

// set by FormatExtension
@Input
public boolean paddedCell = false;
@InputFiles
@OutputFiles
Copy link

Choose a reason for hiding this comment

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

You will have to split the task into two classes:

  • a CheckFormat task
  • an ApplyFormat task

Only the CheckFormat task can be up-to-date checked. The ApplyFormat task changes its own inputs, so it can't be cached/up-to-date checked. Also, if the ApplyFormat task declared the inputs as an output, then cleanApplyFormat would delete your source code ;)

@SkipWhenEmpty
public Iterable<File> target;
@Input
Copy link

Choose a reason for hiding this comment

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

When you have a List of complex objects as an input, these objects must:

  • implement Serializable so Gradle can write them to the cache
  • implement equals/hashcode, so Gradle can compare them to the previous value

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if FormatterStep can even be serialized, as all the impls contain a reference to a Throwing.Function and/or another functional interface...

@nedtwigg @oehme Do you have any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

One idea (I think from @oehme in the issue discussion) is looking at the bytecode of the function, but I'm inclined to declare bankruptcy on input properties being clean w.r.t. incremental compilation. That leaves two options:

A) spotlessCheck will be wrong sometimes after you've changed your format
B) Have three tasks: spotlessCheck/spotlessApply and spotlessFastCheck. check/apply can use the same dumb logic from before, and fastCheck will be faster (using incremental stuff) but user beware that it might be wrong if you're using a custom function that you've changed.

I see Spotless' priorities as such:

  1. Be correct and repeatable.
  2. Make it easy to write a one-liner function to tweak a formatting issue.
  3. Be as fast as possible.

Based on these priorities, I'm inclined to go with option B. It lets us implement spotlessFastCheck without making a full correctness guarantee. We can add a section to the readme Speeding up Spotless which lists the spotlessCheckFast function and lists any provisos, e.g.

spotlessCheckFast will be incorrect if you just changed the format rules. But if the rules have not changed, then it will be much faster than plain-old spotlessCheck.

If we get it to the point that we don't need an asterisk around correctness, then we can dump the old spotlessCheck entirely and promote spotlessCheckFast to the sole check task.

Copy link

@oehme oehme Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure nobody would use a spotlessFastCheck if you have to run spotlessCheck to be sure it's correct anyway.

The only real alternative I see is properly supporting up-to-date checks. Instead of referencing Throwing.Function directly, we should reference a serializable sub-interface of it. Maybe call it FormatterFunction. Then implement the default functions (like regex) in such a way that they properly support equals/hashcode too.

The worst thing that could happen after that is that the task is out-of-date unnecessarily when users use inline steps. The readme about "Making spotless faster" could then explain that users should write real classes with equals/hashcode instead to make the task fast.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's easy to implement all of Spotless' existing formatters in a serializable way. But I don't see an easy way to do this for custom user functions. #46 is a great example of why these custom functions are so useful, and why it's important for them to be easy.

We've got this custom function up-to-date wrinkle, and we can push it onto users in two ways:

A) We can make custom rule-writers deal with it

You can write a custom rule by implementing such-and-such class and then calling the constructor here, and if you change the rule be sure to change the serialVersionUID

B) We can make people who are optimizing their build deal with it

If you want to speed up spotlessCheck, you can fix it with spotlessFastCheck. Here are the provisos:

If somebody's code has formatting problems, it's probably not because running the formatter is too slow, but because it was hard to find or write a formatting rule that did what they want. So I don't think we should take a mild performance issue and turn it into a correctness/customizability issue.

I'm pretty sure nobody would use a spotlessFastCheck if you have to run spotlessCheck to be sure it's correct anyway.

It would still be very useful for --continuous. And imo, that's the only place this optimization makes sense anyway. spotlessCheck is much faster than even a very small unit test suite. And if we're able to remove this wrinkle later, then spotlessFastCheck is better than spotlessCheck in every way, and we can promote it to the job of sole-checker at that time.

Copy link

Choose a reason for hiding this comment

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

You can write a custom rule by implementing such-and-such class and then calling the constructor here, and if you change the rule be sure to change the serialVersionUID

I think you misunderstood my point above.

  • if the author implements an inline rule it will always be out-of-date and thus correct
  • if the author wants extra performance, they can write a class with equals/hashcode

This gives you the best of both worlds: Ease of use by default, performance on demand. No need for a 2-task solution.

Copy link

Choose a reason for hiding this comment

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

And imo, that's the only place this optimization makes sense anyway.

This is only true in very small projects. In a large project, when you change a single module, you want the other modules to be up-to-date, not spend time re-executing the formatter.

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh. Thanks for clarification, I misunderstood. In the approach you describe, a spotlessCheckFast would still be valuable for the --continuous case where you have custom rules which don't implement the full up-to-date contract.

But the simplicity of having only a single "check" class is very valuable, and it's a great feature for all tasks to always be correct. My gut is quite unhappy that Spotless' formatters are no longer just Function<String, String>, but I agree this would be a big speedup for large projects. You've convinced me :) I've got an idea for an easy and mostly-correct way to handle simple rules, which I've described in #47.

@SkipWhenEmpty
public List<FormatterStep> steps = new ArrayList<>();

// set by plugin
@Input
public boolean check = false;

/** Returns the name of this format. */
Expand All @@ -53,32 +70,41 @@ public String getFormatName() {
}

@TaskAction
public void format() throws Exception {
public void format(IncrementalTaskInputs inputs) throws Exception {
if (target == null) {
throw new GradleException("You must specify 'Iterable<File> toFormat'");
}
// combine them into the master formatter
Formatter formatter = new Formatter(lineEndingsPolicy, encoding, getProject().getProjectDir().toPath(), steps);
Formatter formatter = Formatter.builder()
.lineEndingPolicy(lineEndingPolicy)
.encoding(encoding)
.projectDirectory(getProject().getProjectDir().toPath())
.steps(steps)
.build();

// perform the check
if (check) {
formatCheck(formatter);
formatCheck(formatter, inputs);
} else {
formatApply(formatter);
formatApply(formatter, inputs);
}
}

/** Checks the format. */
private void formatCheck(Formatter formatter) throws IOException {
private void formatCheck(Formatter formatter, IncrementalTaskInputs inputs) throws IOException {
List<File> problemFiles = new ArrayList<>();

for (File file : target) {
inputs.outOfDate(input -> {
File file = input.getFile();
getLogger().debug("Checking format on " + file);
// keep track of the problem toFormat
if (!formatter.isClean(file)) {
problemFiles.add(file);
}
}
Errors.rethrow()
Copy link

Choose a reason for hiding this comment

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

Can you give a short explanation why this wrapping needed now?

Copy link
Member Author

@jbduncan jbduncan Oct 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's to do with how, in Java 8, lambdas cannot throw checked exceptions unless the interface they target declares that it throws Throwable. IncrementalTaskInputs.outOfDate(Action) accepts a Groovy Action, which doesn't throw Throwable, so all checked exceptions thrown within the Action have to be wrapped in unchecked exceptions. Therefore, since !formatter.isClean(file) throws IOException - a checked exception - it has to be wrapped in an unchecked exception of sorts, otherwise it won't compile.

In this case, I decided to use Durian's Errors.rethrow() mechanism to wrap all IOExceptions as Errors.WrappedAsRuntimeExceptions, although I could have just as easily wrapped them in UncheckedIOExceptions instead (but that would arguably be less readable, as it would have required try { ... } catch(IOException e) { throw new UncheckedIOException(e); }).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, that was a longer explanation than I expected to write. :)

Copy link

Choose a reason for hiding this comment

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

Thanks, makes perfect sense. Didn't know that method threw a checked exception. It could possibly be changed to use an unchecked exception further down.

.wrap(() -> {
if (!formatter.isClean(file)) {
problemFiles.add(file);
}
}).run();
});

if (paddedCell) {
PaddedCellTaskMisc.check(this, formatter, problemFiles);
Expand All @@ -100,15 +126,19 @@ GradleException formatViolationsFor(Formatter formatter, List<File> problemFiles
}

/** Applies the format. */
private void formatApply(Formatter formatter) throws IOException {
for (File file : target) {
private void formatApply(Formatter formatter, IncrementalTaskInputs inputs) throws IOException {
inputs.outOfDate(input -> {
File file = input.getFile();
getLogger().debug("Applying format to " + file);
// keep track of the problem toFormat
if (paddedCell) {
PaddedCellTaskMisc.apply(this, formatter, file);
} else {
formatter.applyFormat(file);
}
}
Errors.rethrow()
.wrap(() -> {
if (paddedCell) {
PaddedCellTaskMisc.apply(this, formatter, file);
} else {
formatter.applyFormat(file);
}
}).run();
});
}
}
56 changes: 50 additions & 6 deletions src/main/java/com/diffplug/gradle/spotless/Formatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,68 @@
import org.gradle.api.logging.Logging;

/** Formatter which performs the full formatting. */
public class Formatter {
public final class Formatter {
final LineEnding.Policy lineEndingPolicy;
final Charset encoding;
final Path projectDirectory;
final List<FormatterStep> steps;
final Logger logger = Logging.getLogger(Formatter.class);

private static final Logger logger = Logging.getLogger(Formatter.class);

/** It's important to specify the charset. */
@Deprecated
public Formatter(LineEnding.Policy lineEndingPolicy, Path projectDirectory, List<FormatterStep> steps) {
this(lineEndingPolicy, StandardCharsets.UTF_8, projectDirectory, steps);
}

/**
* The number of required parameters is starting to get difficult to use. Use
* {@link Formatter#builder()} instead.
*/
@Deprecated
public Formatter(LineEnding.Policy lineEndingPolicy, Charset encoding, Path projectDirectory, List<FormatterStep> steps) {
this.lineEndingPolicy = Objects.requireNonNull(lineEndingPolicy);
this.encoding = Objects.requireNonNull(encoding);
this.projectDirectory = Objects.requireNonNull(projectDirectory);
this.steps = new ArrayList<>(steps);
this.lineEndingPolicy = Objects.requireNonNull(lineEndingPolicy, "lineEndingPolicy");
this.encoding = Objects.requireNonNull(encoding, "encoding");
this.projectDirectory = Objects.requireNonNull(projectDirectory, "projectDirectory");
this.steps = new ArrayList<>(Objects.requireNonNull(steps, "steps"));
}

public static Formatter.Builder builder() {
return new Formatter.Builder();
}

public static class Builder {
// required parameters
private LineEnding.Policy lineEndingPolicy;
private Charset encoding;
private Path projectDirectory;
private List<FormatterStep> steps;

private Builder() {}

public Builder lineEndingPolicy(LineEnding.Policy lineEndingPolicy) {
this.lineEndingPolicy = lineEndingPolicy;
return this;
}

public Builder encoding(Charset encoding) {
this.encoding = encoding;
return this;
}

public Builder projectDirectory(Path projectDirectory) {
this.projectDirectory = projectDirectory;
return this;
}

public Builder steps(List<FormatterStep> steps) {
this.steps = steps;
return this;
}

public Formatter build() {
return new Formatter(lineEndingPolicy, encoding, projectDirectory, steps);
}
}

/** Returns true iff the given file's formatting is up-to-date. */
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/com/diffplug/gradle/spotless/PaddedCellTaskMisc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,13 @@ static void check(FormatTask task, Formatter formatter, List<File> problemFiles)

// "fake" Formatter which can use the already-computed result of a PaddedCell as
Step paddedCellStep = new Step();
Formatter paddedFormatter = new Formatter(
formatter.lineEndingPolicy, formatter.encoding, formatter.projectDirectory,
Collections.singletonList(paddedCellStep));
Formatter paddedFormatter =
Formatter.builder()
.lineEndingPolicy(formatter.lineEndingPolicy)
.encoding(formatter.encoding)
.projectDirectory(formatter.projectDirectory)
.steps(Collections.singletonList(paddedCellStep))
.build();

// empty out the diagnose folder
Path diagnoseDir = diagnoseDir(task);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/diffplug/gradle/spotless/DiffMessageFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ private FormatTask create(File... files) {
private FormatTask create(List<File> files) {
Project project = ProjectBuilder.builder().withProjectDir(folder.getRoot()).build();
FormatTask task = project.getTasks().create("underTest", FormatTask.class);
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
task.check = true;
task.target = files;
return task;
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/com/diffplug/gradle/spotless/FormatTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ public void createTask() {
@Test(expected = GradleException.class)
public void testLineEndingsCheckFail() throws IOException {
task.check = true;
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
task.target = Collections.singleton(createTestFile("testFile", "\r\n"));
task.execute();
}

@Test
public void testLineEndingsCheckPass() throws IOException {
task.check = true;
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
task.target = Collections.singleton(createTestFile("testFile", "\n"));
task.execute();
}
Expand All @@ -57,7 +57,7 @@ public void testLineEndingsApply() throws IOException {
File testFile = createTestFile("testFile", "\r\n");

task.check = false;
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
task.target = Collections.singleton(testFile);
task.execute();

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class Bundle {
private FormatTask create(String name, FormatterStep step, boolean check) {
FormatTask task = project.getTasks().create("spotless" + SpotlessPlugin.capitalize(name) + (check ? "Check" : "Apply"), FormatTask.class);
task.steps.add(step);
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
task.check = check;
task.target = Collections.singletonList(file);
return task;
Expand Down
15 changes: 9 additions & 6 deletions src/test/java/com/diffplug/gradle/spotless/PaddedCellTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import java.util.Collections;
import java.util.List;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;

import org.junit.Assert;
import org.junit.Rule;
Expand All @@ -43,14 +42,18 @@ private void misbehaved(Throwing.Function<String, String> step, String input, Pa
testCase(step, input, expectedOutputType, steps, canonical, true);
}

private void wellbehaved(Throwing.Function<String, String> step, String input, PaddedCell.Type expectedOutputType, String canonical) throws IOException {
private void wellBehaved(Throwing.Function<String, String> step, String input, PaddedCell.Type expectedOutputType, String canonical) throws IOException {
testCase(step, input, expectedOutputType, canonical, canonical, false);
}

private void testCase(Throwing.Function<String, String> step, String input, PaddedCell.Type expectedOutputType, String expectedSteps, String canonical, boolean misbehaved) throws IOException {
List<FormatterStep> formatterSteps = new ArrayList<>();
formatterSteps.add(FormatterStep.create("step", step));
Formatter formatter = new Formatter(LineEnding.UNIX_POLICY, StandardCharsets.UTF_8, folder.getRoot().toPath(), formatterSteps);
Formatter formatter = Formatter.builder()
.lineEndingPolicy(LineEnding.UNIX_POLICY)
.encoding(StandardCharsets.UTF_8)
.projectDirectory(folder.getRoot().toPath())
.steps(formatterSteps).build();

File file = folder.newFile();
Files.write(file.toPath(), input.getBytes(StandardCharsets.UTF_8));
Expand All @@ -59,7 +62,7 @@ private void testCase(Throwing.Function<String, String> step, String input, Padd
Assert.assertEquals(misbehaved, result.misbehaved());
Assert.assertEquals(expectedOutputType, result.type());

String actual = result.steps().stream().collect(Collectors.joining(","));
String actual = String.join(",", result.steps());
Assert.assertEquals(expectedSteps, actual);

if (canonical == null) {
Expand All @@ -74,8 +77,8 @@ private void testCase(Throwing.Function<String, String> step, String input, Padd

@Test
public void wellBehaved() throws IOException {
wellbehaved(input -> input, "CCC", CONVERGE, "CCC");
wellbehaved(input -> "A", "CCC", CONVERGE, "A");
wellBehaved(input -> input, "CCC", CONVERGE, "CCC");
wellBehaved(input -> "A", "CCC", CONVERGE, "A");
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/diffplug/gradle/spotless/ResourceHarness.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ protected void assertTask(Consumer<FormatExtension> test, String before, String
// create the task
FormatTask task = createTask(test);
// force unix line endings, since we're passing in raw strings
task.lineEndingsPolicy = LineEnding.UNIX.createPolicy();
task.lineEndingPolicy = LineEnding.UNIX.createPolicy();
// create the test file
File testFile = folder.newFile();
Files.write(testFile.toPath(), before.getBytes(StandardCharsets.UTF_8));
Expand Down