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

Extension of FormatterFunc accepting source file as parameter. #290

Merged
merged 2 commits into from
Sep 2, 2018

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Aug 30, 2018

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

Finalized WTP integration into lib-extra.
@fvgh
Copy link
Member Author

fvgh commented Aug 30, 2018

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.

@fvgh fvgh requested a review from nedtwigg August 31, 2018 07:58
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

(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
Copy link
Member

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
Copy link
Member

nedtwigg commented Aug 31, 2018

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.

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);
}

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

@fvgh
Copy link
Member Author

fvgh commented Sep 1, 2018

  • 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
Copy link
Member

jbduncan commented Sep 1, 2018

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.

@nedtwigg nedtwigg merged commit 91014e9 into master Sep 2, 2018
@nedtwigg nedtwigg deleted the formatter_func_extension branch September 2, 2018 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants