Providing explicit support for Groovy source code (fixes #13) #94

fvgh · 2017-03-20T19:57:04Z

Supporting dedicated configuration for groovy source.
The configuration allows access to formatter steps which have been introduced for Java, but which are also useable for Groovy code.
Integrates Groovy-Eclipse based Groovy formatter step implementation.
Allows coexistent configuration of Java and Groovy formatter steps.

Supporting dedicated configuration for groovy source. The configuration allows access to formatter steps which have been introduced for Java, but which are also useable for Groovy code. Integrates Groovy-Eclipse based Groovy formatter step implementation. Allows coexistent configuration of Java and Groovy formatter steps.

fvgh · 2017-03-20T20:14:21Z

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.

nedtwigg · 2017-03-31T22:15:36Z

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 :)

jbduncan

I've had a very brief look through this PR. Nothing about the overall structure strikes out to me as needing attention, so I'm okay with how it looks so far.

What I'm concerned about though is that, AFAICT, there's no explicit documentation yet on what options one can use to configure a groovy.GrEclipseFormatterStep. I understand that one can pass settings files to a step, but it's not clear to me what settings that a settings file can have and what the syntax is.

Has any documentation for this been written yet? I'd not, I think it would be worth creating a new PR with some basic instructions somewhere.

jbduncan · 2017-04-01T00:47:37Z

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. :)

fvgh · 2017-04-01T09:35:24Z

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.

…Set<String> has complicated serialization. Reused FileSignature.toSortedSet() for a more stable serialization. Also, using FileSignature.toSortedSet() without first calling asNonNulList() would be an error. So, might as well have toSortedSet() call that method itself as its first argument.

…ion its own invisible <a> tag for stable links.

nedtwigg · 2017-04-03T09:36:43Z

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!

jbduncan

Hi @fvgh and @nedtwigg, I managed to find a few relatively minor things that I think could be improved upon, if time and willingness allows. :)

jbduncan · 2017-04-03T09:46:07Z

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

- public static SerializableFileFilter skipFilesNamed(String name) {
- return new SerializableFileFilterImpl.SkipFilesNamed(name);
+ /** Creates a FileFilter which will accept all files except files with the given name(s). */
+ public static SerializableFileFilter skipFilesNamed(String... names) {


I'm not sure if this change is binary-compatible. Do we care about binary compatibility for public types in lib, @nedtwigg?

It's definitely source-compatible, if not binary. Good enough while we've got zero users besides ourselves. Good catch though 👍

jbduncan · 2017-04-03T09:48:39Z

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

+ return false;
+ }
+ }
+ return true;


This could be simplified to !Arrays.asList(namesToSkip).contains(pathname.getName()), I believe.

It could be, at the cost of a teensy-weensy allocation for every time we apply a step. Definitely premature optimization, but the long form is still easy to read.

jbduncan · 2017-04-03T09:51:46Z

plugin-gradle/README.md

+ greclipseFormatFile 'greclipse.properties'
+ }
+}
+```


@nedtwigg, what do you think about touching up the comments in this code block as well?

I'm for it. What comments do you want to touch up?

I want to touch them up such that the code block looks like this:

spotless { java { licenseHeaderFile 'spotless.license.java' googleJavaFormat() // use a specific formatter for Java files } groovy { licenseHeaderFile 'spotless.license.java' excludeJava // excludes all Java sources within the Groovy source dirs from formatting // the Groovy Eclipse formatter extends the Java Eclipse formatter, // so it formats Java files by default (unless `excludeJava` is used). greclipseFormatFile 'greclipse.properties' } }

What do you think?

If you think this change is good, would you kindly push it for me? :)

Thank you. 👍

jbduncan · 2017-04-03T09:52:51Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GroovyExtension.java

+
+public class GroovyExtension extends FormatExtension {
+ public static final String NAME = "groovy";
+ public static final boolean EXCLUDE_JAVA_DEFAULT = false;


I believe NAME could be made package-private and EXCLUDE_JAVA_DEFAULT could be made private, if time allows.

Good catch :) I'll fix.

nedtwigg · 2017-04-03T12:18:10Z

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.

…and SpotlessExtension. This means that Spotless can now use greclipse to format its gradle files.

nedtwigg · 2017-04-03T12:42:31Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java

+ if (eclipseFormatFiles.length == 0) {
+ eclipseFormatFiles = new Object[]{eclipseVersion};
+ eclipseVersion = EclipseFormatterStep.defaultVersion();
+ }
 Project project = getProject();


This is naaasty. Here's a better way, maybe:

public class JavaExtension { public EclipseCfg eclipse() { return eclipse(EclipseFormatterStep.defaultVersion()) } public EclipseCfg eclipse(String version) { return new EclipseCfg(version); } // see https://github.com/diffplug/spotless/blob/a8ab1c1331376e57e63441417316139198c285de/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java#L38-L65 for implementation of EclipseCfg public void eclipseFormatFile(Object fileArg) { warning("Deprecated in favor of 'eclipse().configFile(fileArg)'") eclipse().configFiles(fileArgs) } public void eclipseFormatFile(String version, Object fileArg) { warning("Deprecated in favor of 'eclipse('" + version + "').configFile(fileArg)") eclipse(version).configFiles(fileArgs) } }

The advantage of this approach is it gets all of our DSL on the Optional version argument -> config builder pattern. The downside is it deprecates our main usecase for the sole purpose of accommodating the multiple-config-file-in-groovy usecase.

I'm inclined to avoid the question altogether by only allowing a single config file for both JavaExtension and GroovyExtension. It makes the docs simpler, and users can always concatenate groovy settings themselves. @fvgh how much will this limitation hurt GroovyExtension users?

I'm open to the deprecation path discussed above, but I don't have the time to do the proper testing, update the docs, etc.

@fvgh on an unrelated note, your ext-spotless-greclipse is in jcenter, but not mavencentral. I'm gonna wait til we're sure we've got it right to sync to mavencentral.

How about replacing format file varargs...

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

...with two separate single-argument and iterable versions?

public void greclipseFormatFile(String greclipseVersion, Object greclipseFormatFile) { ... } public void greclipseFormatFile(String greclipseVersion, Iterable<Object> greclipseFormatFiles) { ... }

...or a version where we require at least one format file?

public void greclipseFormatFile(String greclipseVersion, Object firstGreclipseFormatFile, Object... otherGreclipseFormatFiles) {

greclipseFormatFile 'mycfg1.prop', 'mycfg2.prop'. This will break all of your workarounds, I believe. It also breaks the workaround I've implemented in the previous commit.

I can see how it would break my single-arg-and-iterable workaround, but not for my other one.

I'm not really familiar with Groovy, but if it were Java, I believe this would be syntactically correct if used with my latter workaround:

greclipseFormatFile('mycfg1.prop', 'mycfg2.prop');

Unless, are we thinking of "breakage" in terms of "'mycfg1.prop' will still be interpreted as the greclipseVersion rather than the 1st greclipseFormatFile"? (In which case, I'd agree.)

My example above is from the README (loosely).

nedtwigg · 2017-04-03T13:00:04Z

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.

fvgh · 2017-04-03T20:24:02Z

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

Refactor the groovy step, documentation, ... as follows greclipseFormat([version]).configFile a.properties b.xml
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?

nedtwigg · 2017-04-03T20:50:12Z

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.

The configuration allows access to formatter steps which have been introduced for Java, but which are also useable for Groovy code. Integrates Groovy-Eclipse based Groovy formatter step implementation. Allows coexistent configuration of Java and Groovy formatter steps.

jbduncan · 2017-04-05T19:33:45Z

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

fvgh · 2017-04-05T19:46:45Z

@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.

fvgh · 2017-04-05T19:49:15Z

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.

fvgh · 2017-04-05T19:58:49Z

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.

fvgh · 2017-04-05T20:22:54Z

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.

fvgh · 2017-04-05T20:29:37Z

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

jbduncan · 2017-04-05T20:35:59Z

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

fvgh · 2017-04-08T16:42:58Z

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

Hi @fvgh, I have requested some changes regarding your latest commit. Would you kindly look through them for me?

jbduncan · 2017-04-08T16:26:55Z

build.gradle

@@ -1,5 +1,5 @@
 buildscript {
- repositories { maven { url 'https://plugins.gradle.org/m2/' } }
+ repositories { maven { url 'https://plugins.gradle.org/m2/' }}


This should be formatted like the following (if possible):

repositories { maven { . . . } }

That should work. The formatter should accept the format.

Sorry, but the formatter always replaces nested closures by } } and then starting the ping-pong, unless I reformat it to }}.
Furthermore it cannot convinced to leave closures with one entry split over multiple lines.

jbduncan · 2017-04-08T16:30:40Z

gradle/java-publish.gradle

@@ -20,7 +20,7 @@ javadoc {
 // use markdown in javadoc
 def makeLink = { url, text -> "<a href=\"${url}\" style=\"text-transform: none;\">${text}</a>" }
 def javadocInfo = '<h2>' + makeLink("https://github.com/${org}/${name}", "${group}:${project.ext.artifactId}:${ext.version}") +
-' by ' + makeLink('http:https://www.diffplug.com', 'DiffPlug') + '</h2>'
+ ' by ' + makeLink('http:https://www.diffplug.com', 'DiffPlug') + '</h2>'


Nice change! 👍

jbduncan · 2017-04-08T16:30:59Z

gradle/java-publish.gradle

+ username = cred('nexus_user')
+ password = cred('nexus_pass')
+ }
+ }}


It should be formatted like this (if possible):

repositories { maven { . . . } }

jbduncan · 2017-04-08T16:31:12Z

gradle/java-setup.gradle

@@ -38,7 +34,8 @@ eclipseResourceFilters {
 apply plugin: 'findbugs'
 findbugs {
 toolVersion = VER_FINDBUGS
- sourceSets = [sourceSets.main] // don't check the test code
+ sourceSets = [
+ sourceSets.main] // don't check the test code


You should manually reformat this line like this (if possible):

sourceSets = [ sourceSets.main // don't check the test code ]

For array I have only a max length. For example with groovy.formatter.longListLength=1, we can assure that all entries are in separate lines. Hence we can archive:

sourceSets = [ // don't check the test code sourceSets.main ]

jbduncan · 2017-04-08T16:32:29Z

lib-extra/build.gradle

@@ -22,6 +22,5 @@ dependencies {
 }

 // we'll hold the core lib to a high standard
-findbugs {
- reportLevel = 'low' // low|medium|high (low = sensitive to even minor mistakes)
+findbugs { reportLevel = 'low' // low|medium|high (low = sensitive to even minor mistakes)
 }


It's not clear to me why Greclipse formatted this section like this.

Can we prevent it with a setting?

I think it is the comment, which is in the same line. The Groovy-Formatter seems only to accept this or method calls.

Hmm, that's a shame.

Could we work around this problem with the following manual formatting?

findbugs { reportLevel = 'low' } // low|medium|high (low = sensitive to even minor mistakes)

Yes, that works.

jbduncan · 2017-04-08T16:32:47Z

lib/build.gradle

@@ -12,6 +12,5 @@ dependencies {
 }

 // we'll hold the core lib to a high standard
-findbugs {
- reportLevel = 'low' // low|medium|high (low = sensitive to even minor mistakes)
+findbugs { reportLevel = 'low' // low|medium|high (low = sensitive to even minor mistakes)
 }


It's not clear to me why Greclipse formatted this section like this.

Can we prevent it with a setting?

Yes, by rearranging the position of the comment.

Okay, cool. 👍

jbduncan · 2017-04-08T16:38:30Z

spotlessSelf.gradle

+ include '**/*.gradle'
+ exclude '_ext/**'
+ }
+ custom 'preventFormatPingPong', { return it.replaceAll('}[ \t\f]+}', '}}') }


Please refactor the closure of the custom 'noInternalDeps', { ... } step in the java { ... } block above, into a variable, and pass that variable as a custom step to both the java and groovy blocks, like so:

spotless { def noInternalDepsClosure = { source -> if (source.contains('import org.gradle.internal.') { throw new AssertionError("Accidental internal import") } } java { . . . custom 'noInternalDeps', noInternalDepsClosure . . . } groovy { . . . custom 'noInternalDeps', noInternalDepsClosure . . . } }

jbduncan · 2017-04-08T16:39:49Z

spotlessSelf.gradle

+ include '**/*.gradle'
+ exclude '_ext/**'
+ }
+ custom 'preventFormatPingPong', { return it.replaceAll('}[ \t\f]+}', '}}') }


And then please add the following line just below

bumpThisNumberIfACustomStepChanges(1)

jbduncan · 2017-04-08T16:40:40Z

spotlessSelf.gradle

+ include '**/*.gradle'
+ exclude '_ext/**'
+ }
+ custom 'preventFormatPingPong', { return it.replaceAll('}[ \t\f]+}', '}}') }


I'm mildly curious by the \f here, as it's a unusual regex pattern for me. What does it match against?

Would matching against \t as well be sensible here?

The \t should be sensible, since it will most likely to the ping-pong as well. But yes the \f should be removed.

jbduncan · 2017-04-08T16:51:12Z

testlib/build.gradle

@@ -13,6 +13,5 @@ dependencies {
 }

 // we'll hold the testlib to a low standard (prize brevity)
-findbugs {
- reportLevel = 'high' // low|medium|high (low = sensitive to even minor mistakes)
+findbugs { reportLevel = 'high' // low|medium|high (low = sensitive to even minor mistakes)
 }


It's not clear to me why Greclipse formatted this section like this.

Can we prevent it with a setting?

No, just by rearranging the position of the comment.

fvgh · 2017-04-08T17:08:30Z

@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?

jbduncan · 2017-04-08T17:13:47Z

@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. :)

… greclipse formatter)

jbduncan

Thanks @fvgh! Two minor things left.

jbduncan · 2017-04-08T18:46:41Z

spotlessSelf.gradle

@@ -6,18 +6,19 @@ plugins {

 repositories { jcenter() }
 spotless {
+ def noInternalDepsClosure = {
+ if (it.contains('import org.gradle.internal.') &&
+ !it.contains('def noInternalDepsClosure')) {


I think this condition does little if anything to help here.

With this condition in place, it means that if we format spotlessSelf.gradle, then it will never throw AssertionError even if it imports org.gradle.internal.*. I don't know if build files can even import Gradle-internal packages, but we might as well play it safe I reckon.

Oh, I'm most likely mistaken!

I can see that if !it.contains('def noInternalDepsClosure') is not in place, then running spotlessSelf.gradle on itself would cause an AssertionError.

Never mind! 😝

jbduncan · 2017-04-08T18:47:53Z

spotlessSelf.gradle

@@ -28,7 +29,9 @@ spotless {
 include '**/*.gradle'
 exclude '_ext/**'
 }
+ custom 'noInternalDeps', noInternalDepsClosure
 custom 'preventFormatPingPong', { return it.replaceAll('}[ \t\f]+}', '}}') }


Are you still working on replacing \f with \t?

…expected formatter output.

fvgh · 2017-04-09T06:39:06Z

@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.

fvgh · 2017-04-09T09:01:45Z

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

…ct to test. Omit implicit testing of end-line characters for licenseHeader step.

EXCLUDE_JAVA_DEFAULT was being used as an alias for `false`. If we changed its value to true, it didn't just change the default behavior, it would also break the implementation logic.

…enCentral as well.

…an reference it directly.

The DSL should be based on the name of the project. Many formatters have "format" in the name -> google-java-format, scalafmt, etc. But others don't, e.g. ktlint.

nedtwigg · 2017-04-09T21:16:59Z

README.md

@@ -77,6 +77,7 @@ lib('scala.ScalaFmtStep') +'{{yes}} | {{no}}
 + implementing up-to-date checking [#31](https://github.com/diffplug/spotless/issues/31)
 + breaking spotless into libraries [#56](https://github.com/diffplug/spotless/issues/56)
 + lots of other things, but especially the diff support in `spotlessCheck`
+* Huge thanks to [Frank Vennemeyer](https://github.com/fvgh) for [Groovy support via greclipse](https://github.com/diffplug/spotless/issues/13).
 * Huge thanks to [Stefan Oehme](https://github.com/oehme) for tons of help on the internal mechanics of Gradle.


@fvgh do you have a different homepage you'd like me to link to?

Thanks, the page is fine.

nedtwigg · 2017-04-09T21:19:04Z

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!

nedtwigg · 2017-04-09T21:22:12Z

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().

fvgh · 2017-04-10T07:08:42Z

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.

nedtwigg · 2017-04-10T14:59:14Z

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!

nedtwigg · 2017-04-10T14:59:21Z

Errr, 3.3.0

… discussed in #94.

jbduncan reviewed Mar 31, 2017

View reviewed changes

nedtwigg added 2 commits April 3, 2017 00:35

Fixed some grammar and spelling errors in README. Also gave each sect…

0628870

…ion its own invisible <a> tag for stable links.

jbduncan reviewed Apr 3, 2017

View reviewed changes

nedtwigg added 4 commits April 3, 2017 03:46

Reduced visibility of GroovyExtension.NAME and EXCLUDE_JAVA_DEFAULT.

dbfb2c3

Improved formatting for the gradle plugin README block.

115b987

Publish ext-greclipse 2.3.0.

c8377d1

Now that greclipse is in jcenter, we can strip out the snapshot stuff.

97f06c3

Added a workaround for the new varargs ambiguity for GroovyExtension …

196c29d

…and SpotlessExtension. This means that Spotless can now use greclipse to format its gradle files.

nedtwigg reviewed Apr 3, 2017

View reviewed changes

Fixed greclipseFormat ping/pong. Provided proposal for format.

b9c1358

jbduncan requested changes Apr 8, 2017

View reviewed changes

Fixes according to review by @jbduncan (as far as possible, using the…

4becbed

… greclipse formatter)

jbduncan requested changes Apr 8, 2017

View reviewed changes

fvgh added 2 commits April 8, 2017 20:53

As agreed in previous review: Femoval of unnecessary '\f'

cdf45fc

Fixed description/values of Groovy-Eclipse configuration example and …

25d57ad

…expected formatter output.

jbduncan approved these changes Apr 9, 2017

View reviewed changes

fvgh and others added 7 commits April 9, 2017 15:19

Make GroovyDefaultTargetTest platform independent. Focus on the subje…

b24bf3f

…ct to test. Omit implicit testing of end-line characters for licenseHeader step.

Changed excludeJava to use the same idiom we use for paddedCell().

8fd9a2f

EXCLUDE_JAVA_DEFAULT was being used as an alias for `false`. If we changed its value to true, it didn't just change the default behavior, it would also break the implementation logic.

ext-greclipse is now in mavenCentral, so the tests can be against mav…

cc69b88

…enCentral as well.

Rather than copy-pasting JavaExtension.LICENSE_HEADER_DELIMITER, we c…

f6c1c7a

…an reference it directly.

Renamed greclipseFormat to just greclipse.

d78a2b6

The DSL should be based on the name of the project. Many formatters have "format" in the name -> google-java-format, scalafmt, etc. But others don't, e.g. ktlint.

Minor corrections to groovy section of plugin-gradle/README.md

5a863b9

Update changelog and congratulate Frank on his contribution!!

027892f

nedtwigg reviewed Apr 9, 2017

View reviewed changes

nedtwigg merged commit 5046667 into diffplug:master Apr 10, 2017

fvgh mentioned this pull request Apr 29, 2017

Check fails after Apply using PaddedCell #105

Closed

fvgh added a commit that referenced this pull request May 5, 2017

Align Java Eclipse formatter gradle interface with greclipse/scala as…

1cd6bbe

… discussed in #94.

fvgh mentioned this pull request May 5, 2017

Align Java Eclipse formatter gradle interface with greclipse/scala #109

Merged

jbduncan pushed a commit that referenced this pull request May 5, 2017

Align Java Eclipse formatter gradle interface with greclipse/scala as…

7aded96

… discussed in #94.

Providing explicit support for Groovy source code (fixes #13) #94

Providing explicit support for Groovy source code (fixes #13) #94

Conversation

fvgh commented Mar 20, 2017

fvgh commented Mar 20, 2017

nedtwigg commented Mar 31, 2017

jbduncan left a comment

Choose a reason for hiding this comment

jbduncan commented Apr 1, 2017

fvgh commented Apr 1, 2017

nedtwigg commented Apr 3, 2017

jbduncan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbduncan Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedtwigg commented Apr 3, 2017

Choose a reason for hiding this comment

jbduncan Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

nedtwigg Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbduncan Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedtwigg commented Apr 3, 2017

fvgh commented Apr 3, 2017 • edited Loading

nedtwigg commented Apr 3, 2017

jbduncan commented Apr 5, 2017

fvgh commented Apr 5, 2017

fvgh commented Apr 5, 2017

fvgh commented Apr 5, 2017

fvgh commented Apr 5, 2017

fvgh commented Apr 5, 2017

jbduncan commented Apr 5, 2017

fvgh commented Apr 8, 2017

jbduncan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvgh Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvgh commented Apr 8, 2017

jbduncan commented Apr 8, 2017 • edited Loading

jbduncan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbduncan Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvgh commented Apr 9, 2017

jbduncan Apr 3, 2017 •

edited

Loading

jbduncan Apr 3, 2017 •

edited

Loading

nedtwigg Apr 3, 2017 •

edited

Loading

jbduncan Apr 3, 2017 •

edited

Loading

fvgh commented Apr 3, 2017 •

edited

Loading

fvgh Apr 8, 2017 •

edited

Loading

jbduncan commented Apr 8, 2017 •

edited

Loading

jbduncan Apr 10, 2017 •

edited

Loading