Extension of FormatterFunc accepting source file as parameter.

[fvgh]

Extension of FormatterFunc accepting source file as parameter.
Finalized WTP integration into lib-extra which has been started in #279 .

[fvgh]

Strange. According to Travis, some Spotless Eclipse formatters cannot be downloaded. For example spotless-ext-greclipse-2.3.0.pom access fails. Have no problem using wget...

Travis Error code is 403. So it's seems to be a Travis proxy authentication issue.
I wrote a corresponding e-mail to the Travis team.

Travis works again. Restarting builds of all affected PRs.

[nedtwigg]

This was done very elegantly.

My first instinct is that it's bad for FormatterFunc to take a File. Allowing access to the File directly is a risky hack, because it means the formatter might read the file from disk, rather than use the "unix input" that is supplied. Reading from disk will break the composability that spotless has currently. That's why the File is currently only actually used by FileFilters.

But the only alternative that I can see is to rebuild all of FormatterStep.createLazy() with a parallel FormatterStepWithFile.createLazy(). But that's a lot of duplicate code - since the code change submitted here is so small, and still encourages FormatterStep authors to use only the String argument because of the default method.

If there were another way, I would definitely prefer that other way, but I can't see it. I would like at least one more opinion from one of the other committers before we merge this though...

[jbduncan]

Reading from disk will break the composability that spotless has currently.

Oh, really? I'm not sure I see why. Can you explain things further for me @nedtwigg? 🤔

[jbduncan]

(Once I better understand why reading from the file directly breaks the composability of Spotless, I'd be more than happy to pitch in my own review!)

[jbduncan]

Wait, am I right to think that reading from the file directly would break composability because at any given time, the supplied "unix input" could be partially formatted and thus inconsistent with the contents of the original file?

[nedtwigg]

EDIT: exactly! Didn't see this comment until I already wrote:

The data flow in spotless is:

• load file from disk
• put unix line endings on it
• pass that to the first step
• pass the result to the next step
• ...
• put the intended line endings back on
• check that it equals the original load / write to disk

The only thing a step knows is what gets passed to it by the previous step. If a step has a file path, it can ignore its input parameter and just load the content straight from disk. Currently, we only ever use the path to skip applying a step - not to actually implement the formatter itself. This PR will be the first time that a formatter implementation is using the file path to decide how to format the file.

spotless/lib/src/main/java/com/diffplug/spotless/Formatter.java

Lines 228 to 242 in 366c1b9

	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);
	}
	} catch (Throwable e) {
	String relativePath = rootDir.relativize(file.toPath()).toString();
	exceptionPolicy.handleError(e, step, relativePath);
	}

[jbduncan]

Similarly to @nedtwigg, I agree that allowing the original file to be read could be bad (if I understand correctly what he meant by breaking "composability".) However, I also cannot see an easy way around this, so I think this PR is actually okay!

I'm also impressed by how elegant the changes are and how small the changes are. So well done @fvgh!

I would have suggested, as a small nit, that the File parameter be changed to be a Path instead. However, I do realise that we use File and Path inconsistently throughout Spotless, I think originally because Gradle depends on File for many of its user-facing APIs including project.file(). So ultimately I don't mind which of File and Path is used.

[jbduncan]

EDIT: exactly! Didn't see this comment until I already wrote:

Cool, thanks @nedtwigg! In that case, my earlier review still stands: consider this PR approved from me. :)

[jbduncan]

@nedtwigg And your comment regarding Spotless's data flow/pipeline makes complete sense to me, so cheers for confirming my understanding for me! 👍

[fvgh]

• Background Info: The reason for change and the "undesired" impact had been discussed in Provision of Spotless Eclipse XML formatter plugin. #230 .
• About Path/File: The FormatterFunc is in my opinion just an interface helper for FormatterStep. Though the usage of Path is more modern, I would prefer the same types in FormatterFunc and FormatterStep.

So all in all, I would propose to stick with the current solution.

[jbduncan]

Though the usage of Path is more modern, I would prefer the same types in FormatterFunc and FormatterStep.

Makes sense to me! I'm happy for File to be used over Path then.