Skip to content

Commit

Permalink
No longer be lenient with FormatterSteps returning null.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbduncan
jbduncan committed Dec 23, 2016
1 parent 45deb18 commit 08673f2
Showing 1 changed file with 2 additions and 8 deletions.
10 changes: 2 additions & 8 deletions lib/src/main/java/com/diffplug/spotless/Formatter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,8 @@ public String applySteps(String unix, File file) throws Error {
for (FormatterStep step : steps) {
try {
String formatted = step.format(unix, file);
if (formatted == null) {
// This probably means it was a step that only checks
// for errors and doesn't actually have any fixes.
// No exception was thrown so we can just continue.
} else {
// Should already be unix-only, but some steps might misbehave.
unix = LineEnding.toUnix(formatted);
}
// Should already be unix-only, but some steps might misbehave.
unix = LineEnding.toUnix(formatted);
} catch (Error e) {
logger.severe("Step '" + step.getName() + "' found problem in '" + rootDir.relativize(file.toPath()) + "':\n" + e.getMessage());
throw e;
Expand Down

5 comments on commit 08673f2

@jbduncan
Copy link
Member Author

Choose a reason for hiding this comment

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

@nedtwigg I wonder if I could have your opinion on whether we should allow users to create FormatterSteps that return null strings.

IMO it doesn't really make sense to allow this, as null doesn't mean anything in this context AFAICT. The way I see it, a null return-value may (1) indicate bug(s) in user FormatterSteps, and (2) be rather error-prone for us, in case we forget that FormatterSteps can return null and we don't write Spotless's code defensively enough.

However, I wanted to hear your thoughts on this, as I think I may have missed some good reasons for allowing FormatterSteps to return @Nullable strings in the first place.

@nedtwigg
Copy link
Member

Choose a reason for hiding this comment

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

The reason for allowing null return values is this issue:

#46

@jbduncan
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 admit I'm still not sure why null is allowed as a valid return value in this case. Is it because Groovy closures themselves return null if they don't return any other value?

@nedtwigg
Copy link
Member

Choose a reason for hiding this comment

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

Correct, they return null. Not a big deal to ask people to return the argument, but also not a big deal to handle null cleanly. I opted to allow null because I had never seen a bug in spotless caused by accidental null return, but I had a usecase in front of me that benefitted from it.

@jbduncan
Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that sounds pretty reasonable to me! I've got a local commit that reverts this one, so I'll start pushing now. :)

Please sign in to comment.