Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Apr 10, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
952fe05
Providing explicit support for Groovy source code (fixes #13)
fvgh Mar 20, 2017
9635398
SkipFilesNamed's equality is based on its serialized representation. …
nedtwigg Apr 3, 2017
0628870
Fixed some grammar and spelling errors in README. Also gave each sec…
nedtwigg Apr 3, 2017
dbfb2c3
Reduced visibility of GroovyExtension.NAME and EXCLUDE_JAVA_DEFAULT.
nedtwigg Apr 3, 2017
115b987
Improved formatting for the gradle plugin README block.
nedtwigg Apr 3, 2017
c8377d1
Publish ext-greclipse 2.3.0.
nedtwigg Apr 3, 2017
97f06c3
Now that greclipse is in jcenter, we can strip out the snapshot stuff.
nedtwigg Apr 3, 2017
196c29d
Added a workaround for the new varargs ambiguity for GroovyExtension …
nedtwigg Apr 3, 2017
4eac44d
Supporting dedicated configuration for groovy source.
fvgh Apr 5, 2017
b9c1358
Fixed greclipseFormat ping/pong. Provided proposal for format.
fvgh Apr 8, 2017
4becbed
Fixes according to review by @jbduncan (as far as possible, using the…
fvgh Apr 8, 2017
cdf45fc
As agreed in previous review: Femoval of unnecessary '\f'
fvgh Apr 8, 2017
25d57ad
Fixed description/values of Groovy-Eclipse configuration example and …
fvgh Apr 9, 2017
b24bf3f
Make GroovyDefaultTargetTest platform independent. Focus on the subje…
fvgh Apr 9, 2017
8fd9a2f
Changed excludeJava to use the same idiom we use for paddedCell().
nedtwigg Apr 9, 2017
cc69b88
ext-greclipse is now in mavenCentral, so the tests can be against mav…
nedtwigg Apr 9, 2017
f6c1c7a
Rather than copy-pasting JavaExtension.LICENSE_HEADER_DELIMITER, we c…
nedtwigg Apr 9, 2017
d78a2b6
Renamed greclipseFormat to just greclipse.
nedtwigg Apr 9, 2017
5a863b9
Minor corrections to groovy section of plugin-gradle/README.md
nedtwigg Apr 9, 2017
027892f
Update changelog and congratulate Frank on his contribution!!
nedtwigg Apr 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GroovyExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ public void greclipseFormatFile(Object... greclipseFormatFiles) {
}

public void greclipseFormatFile(String greclipseVersion, Object... greclipseFormatFiles) {
// disambiguate the ambiguous methods
// see ScalaExtension.scalafmt() for a much better mechanism: https://github.com/diffplug/spotless/blob/a8ab1c1331376e57e63441417316139198c285de/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java#L38-L65
if (greclipseFormatFiles.length == 0) {
greclipseFormatFiles = new Object[]{greclipseVersion};
greclipseVersion = GrEclipseFormatterStep.defaultVersion();
}
Project project = getProject();
addStep(GrEclipseFormatterStep.create(greclipseVersion,
project.files(greclipseFormatFiles).getFiles(),
Expand Down
6 changes: 6 additions & 0 deletions plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ public void eclipseFormatFile(Object eclipseFormatFile) {
}

public void eclipseFormatFile(String eclipseVersion, Object... eclipseFormatFiles) {
// disambiguate the ambiguous methods
// see ScalaExtension.scalafmt() for a much better mechanism: https://github.com/diffplug/spotless/blob/a8ab1c1331376e57e63441417316139198c285de/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java#L38-L65
if (eclipseFormatFiles.length == 0) {
eclipseFormatFiles = new Object[]{eclipseVersion};
eclipseVersion = EclipseFormatterStep.defaultVersion();
}
Project project = getProject();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jbduncan jbduncan Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@nedtwigg nedtwigg Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@jbduncan jbduncan Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

My example above is from the README (loosely).

addStep(EclipseFormatterStep.create(eclipseVersion,
project.files(eclipseFormatFiles).getFiles(),
Expand Down
15 changes: 11 additions & 4 deletions spotlessSelf.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ plugins {
id 'java'
}

repositories { mavenCentral() }
repositories { jcenter() }
spotless {
java {
target '**/*.java'

target fileTree('.') {
include '**/*.java'
exclude '_ext/*/build/**'
}
custom 'noInternalDeps', {
if (it.contains('import org.gradle.internal.')) {
throw new AssertionError("Accidental internal import")
Expand All @@ -21,6 +23,11 @@ spotless {
eclipseFormatFile 'spotless.eclipseformat.xml'
trimTrailingWhitespace()
}
groovy {
target 'build.gradle', 'spotlessSelf.gradle', 'settings.gradle'
greclipseFormatFile 'spotless.eclipseformat.xml'
paddedCell()
}
freshmark {
target '**/*.md'
propertiesFile('gradle.properties')
Expand All @@ -30,7 +37,7 @@ spotless {
}
}
format 'misc', {
target '**/*.gradle', '**/*.md', '**/*.gitignore'
target '**/*.md', '**/*.gitignore'
indentWithTabs()
trimTrailingWhitespace()
endWithNewline()
Expand Down