GeezerVison

Note that I also would like to align afterwards the Java related tests with the once for Groovy. But I think this should be done in a different PR. Maybe I should also split the current PR into changes for GrEclipseFormatterStep and changes for GroovyExtension. Let me know if you want a split.

Very sorry this has languished so long. Conference last week swamped me, this week is catch up. I promise to review this this weekend (and hopefully merge :)

Oh, I just realised that there are instructions within this PR that seem to describe how to configure a GrEclipseFormatterStep, silly me!

I'll have a proper look at them later if/when I have the time. :)

No worries. I had no time to continue with the work myself.
@jbduncan It would be very kind if you can check my changes to the documentation. Sine I am not a native speaker, I would profit from a review / proposals for enhancements.

This LGTM. I've published spotless-ext-greclipse to bintray: https://bintray.com/diffplug/opensource/spotless-ext-greclipse

Currently waiting for inclusion into jcenter, then I can sync to mavencentral. Once we've got the artifact in there, we can change greclipse to not need any strange repositories, merge to master, and publish!

We're sooo close! Have an interesting problem. I tried using the groovy formatter in spotlessSelf.gradle as such:

    java {
        ...
        eclipseFormatFile 'spotless.eclipseformat.xml'
    }
    groovy {
        target 'build.gradle', 'spotlessSelf.gradle', 'settings.gradle'
        greclipseFormatFile 'spotless.eclipseformat.xml'
    }

The problem is that spotless.eclipseformat.xml is being read as a version rather than a file. Why? We've got an ambiguous overload with varargs:

GroovyExtension {
    public void greclipseFormatFile(Object... greclipseFormatFiles) { ... }
    public void greclipseFormatFile(String greclipseVersion, Object... greclipseFormatFiles) { ... }
}

It's copied from the JavaExtension, which used to have these two methods:

JavaExtension {
    public void eclipseFormatFile(Object eclipseFormatFile) { ... }
    public void eclipseFormatFile(String eclipseVersion, Object eclipseFormatFile) { ... }
}

That was unambiguous. But now that we've turned Object formatFile into Object formatFiles..., it's ambiguous, for both java and groovy.

If I could go back in time, I would make all of our versioned plugins work the way that the scala extension works. That pattern leaves much more room for future extensibility without bumping into these kinds of problems.

But I don't want to break the Java DSL for our current users, and it makes sense for the Groovy and Java DSL to be similar. So I think our best approach is the workaround I'm about to upload.

Pretty sure the travis failure is a bug in travis (it's building an old commit and reporting that it's building a new one). I've reported to Travis, hopefully will resolve with future commits.

Sorry @nedtwigg and @jbduncan that you had to put in so much work on this PR.
I will (Wednesday, no time tomorrow)

1. Refactor the groovy step, documentation, ... as follows greclipseFormat([version]).configFile a.properties b.xml
2. Squash all commits and reopen the PR (or shall the history be kept?)

To get a feeling how configuration shall look like:
Ok that we stick with a singular configFile but allowing multiple configuration files? Otherwise I would revert my changes to FreshMark.
Wouldn't it be good to introduce additionally eclipseFormat([version]).configFile?

No apologies!! You've built a great feature that a lot of users want. The original sin here was me building a sloppy escape hatch for setting the version.

In the bad old days, Spotless didn't have Provisioner, and all of its formatters were compile-time dependencies. That didn't scale as we added support for more languages, so we added Provisioner. A nice benefit of this abstraction is that now users can peg the version of their formatter implementation while still getting framework-level improvements, but it means that pre-Provisioner DSL either has no public mechanism for setting the version (FreshMark), or has a janky mechanism (Eclipse).

Refactor the groovy step, documentation, ... as follows greclipseFormat([version]).configFile a.properties b.xml
Wouldn't it be good to introduce additionally eclipseFormat([version]).configFile

These both sound great. It's easy to make a nice warning message for helping legacy eclipseFormatFile users migrate.

Squash all commits and reopen the PR (or shall the history be kept?)

Whatever's easier for you. I don't care if we close this PR and start fresh or whatever. This history is interesting but not critical.

Ok that we stick with a singular configFile but allowing multiple configuration files?

I think so.

Looks like some of the changes @nedtwigg made have been overridden by @fvgh's rebase attempt?

@jbduncan Actually I did not a re-base. I just wanted to squash the commits. Ans since I was on it, I wanted to start at head of master since there were already conflicts reported.
@nedtwigg introduced already the groovy check on the gradle build files. That one is failing, but to me it seems to be a bug in my groovy step. Don't know whether I can solve it today.

I am not sure, but it seems to me the Travis problem @nedtwigg was referring to is not a problem with Travis, but in my code.

Yes confirmed. Just understood what PaddeCell does for me 😄 . Ok, still missing some basics about the project 😞 . Not sure why the problem (have not found the root cause yet) did not show up in the unit-tests. Should have been detected by GrEclipseFormatterStepTest.

Sorry guys. Started to look for red herrings. The basics are sane. Must be some stupidity I inserted in the small refactoring I did for the last commit. Have to call it a day. Must postpone the PR until the weekend.

@jbduncan I will omit the null-checks in this PR till your LibPreconditions.java is available.
Hope that's alright with you.

@fvgh Yep, it's alright with me. 👍

With the last change I introduced a proposal for the greclipsFormat ping-pong handling. The ping-pong is basically a ['C', 'A', 'B', 'A', 'B', ...] series. Hence it could not be solved by paddedCell.

I did not fully research the ping-pong problem. It occurs even with the latest Groovy-Eclipse snapshot (Feb. 2017) in my Eclipse Neon.3. I used the formatter in the Eclipse GUI several times on the input c1 { c2 { f 'p' }} to assure that the problem can be reproduced.
The greclipse I introduced so far is based on a release from 2014. So I waned to check whether an update to a snapshot would solve the issue. But it would not.
It s related to the AST parsing and the new line after braces. It can also be resolved by setting groovy.formatter.braces.end=same, but I don't like the formatted gradle files with this option set.

Since I understood that @nedtwigg wanted too see a demonstration within this PR, I added a proposal in a separated commit. As you will observe, Groovy-Eclipse Formatter is far from the state the JDT formatter is in, but it is the best I know.
I left out the _ext, since the formatter does not accept annotations at the end of a line within arrays. It always introduces a line-break. Unlike for the JDT formatter, this formatting is not configurable.
Let me know whether I shall move it to a separate PR/Issue for now, or if you think the formatter is not suitable for spotless gradle.

@jbduncan So in summary, you think using greclipse for spotless gradle is principally ok? Shall I provide a new proposal within this PR, or shall we split this issue?

@fvgh I think that using Greclipse on Spotless is a good idea! But I'd like to wait until @nedtwigg gets back before we commit to it, as he's the principal decision maker and so he may have other ideas. :)

@nedtwigg Let me know what you would propose about using greclipse to format the gradle files. As soon as we reach a final agreement, I'll do quickly the cherry-picking and squash the commits.

I am afraid the testing for this PR under Windows are not yet successful. Provide a solution later on.

This looks great!!! I'm ready to merge and release. I made a few changes and updated the changelog. Once I get signoff from you that my changes are okay, we can ship it!

Btw, just a note:

The ping-pong is basically a ['C', 'A', 'B', 'A', 'B', ...] series. Hence it could not be solved by paddedCell.

PaddedCell can resolve any ambiguity. For converging sequences, it will just silently let it slide. For cycles and diverging sequences, the user has to manually specify paddedCell().

I squashed the commits.
Thanks again to both of you for the support.
Independent of this PR I will have another look why paddedCell() did not work out for me when I used greclipse.

Merged!! I unsquashed them. We've done some merge commits along the way, so squashing made the history confusing to look at (we still couldn't do a simple rebase). Also, I tagged one of the intermittent commits as the commit which released to mavenCentral. The merge commit does a nice job providing a summary of all the changes.

It should be in oss.snapshots in ~10 minutes, we'll make sure it's doing what we want, and then I'll ship 3.0.0!

Errr, 3.3.0