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.

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

Jump to bottom

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

Merged
merged 1 commit into from
May 5, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion plugin-gradle/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ spotless {

removeUnusedImports() // removes any unused imports

eclipseFormatFile 'spotless.eclipseformat.xml' // XML file dumped out by the Eclipse formatter
eclipse().configFile 'spotless.eclipseformat.xml' // XML file dumped out by the Eclipse formatter
// If you have Eclipse preference or property files, you can use them too.
}
}
Expand Down
49 changes: 45 additions & 4 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 @@ -15,6 +15,8 @@
*/
package com.diffplug.gradle.spotless;

import static com.diffplug.gradle.spotless.PluginGradlePreconditions.requireElementsNonNull;

import java.util.List;
import java.util.Objects;

Expand All @@ -24,6 +26,8 @@
import org.gradle.api.plugins.JavaPluginConvention;
import org.gradle.api.tasks.SourceSet;

import com.diffplug.common.base.StringPrinter;
import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.SerializableFileFilter;
import com.diffplug.spotless.extra.java.EclipseFormatterStep;
import com.diffplug.spotless.generic.LicenseHeaderStep;
Expand Down Expand Up @@ -59,17 +63,23 @@ public void importOrderFile(Object importOrderFile) {
addStep(ImportOrderStep.createFromFile(getProject().file(importOrderFile)));
}

/** Use {@link #eclipse()} instead */
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be easier for a new user to understand if this said,

/** Use {@link #eclipse().configFile(String...)} instead. */

Copy link
Member Author

Choose a reason for hiding this comment

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

That would not be JavaDoc compliant. The comment is more fore the plugin developer. The user gets the "Use 'eclipse([version]).configFile()' instead." warning at the end of the build. Sounds reasonable to you?

Copy link
Member

@jbduncan jbduncan May 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That would not be JavaDoc compliant.

Oh yes, you're right! Silly me. 😉

I do understand that, from a Gradle user perspective, they'd write eclipse([version]).configFile configFile.xml, but I still think that, from a spotless-lib user perspective, it should be clearly stated which method after eclipse() needs to be invoked in the chain to get the exact same behaviour as eclipseFormatFile(), so as to prevent confusion or ambiguity.

How about the following JavaDoc for eclipseFormatFile(String, Object)...

/**
 * Use {@link #eclipse(String) eclipse(String)}{@code .}{@link
 * EclipseConfig#configFile(Object...) configFile(Object...)} instead.
 */

...and the following for eclipseFormatFile(Object) instead?

/**
 * Use {@link #eclipse() eclipse()}{@code .}{@link EclipseConfig#configFile(Object...)
 * configFile(Object...)} instead.
 */

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea itself is very nice and I will definitely use this approach in future when describing more complex use cases.
But honestly I do not think that this would fit here. I saw that you guys try avoid superfluous comments where code is self explaining. I think we agree that the usage of EclipseConfig is quite self explaining.
But that's the final comment from my side. The final decision is yours. Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, @nedtwigg, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I feel it's 50/50 - tie goes to the implementor :)

@Deprecated
public void eclipseFormatFile(Object eclipseFormatFile) {
eclipseFormatFile(EclipseFormatterStep.defaultVersion(), eclipseFormatFile);
}

/** Use {@link #eclipse(String)} instead */
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be easier for a new user to understand if this said,

/** Use {@link #eclipse(String).configFile(String...)} instead. */

Copy link
Member

Choose a reason for hiding this comment

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

Again, 50/50, tie goes to the implementor :)

@Deprecated
public void eclipseFormatFile(String eclipseVersion, Object eclipseFormatFile) {
Objects.requireNonNull(eclipseVersion, "eclipseVersion");
Objects.requireNonNull(eclipseFormatFile, "eclipseFormatFile");
Project project = getProject();
addStep(EclipseFormatterStep.create(eclipseVersion,
project.files(eclipseFormatFile).getFiles(),
GradleProvisioner.fromProject(project)));
getProject().getLogger().warn(
StringPrinter.buildStringFromLines(
"'eclipseFormatFile [version] <file>' is deprecated.",
"Use 'eclipse([version]).configFile(<file>)' instead.",
"For details see https://github.com/diffplug/spotless/tree/master/plugin-gradle#applying-to-java-source"));
eclipse(eclipseVersion).configFile(eclipseFormatFile);
}

/** Removes any unused imports. */
Expand All @@ -93,6 +103,37 @@ public void googleJavaFormat(String version) {
addStep(GoogleJavaFormatStep.create(version, GradleProvisioner.fromProject(getProject())));
}

public EclipseConfig eclipse() {
return eclipse(EclipseFormatterStep.defaultVersion());
}

public EclipseConfig eclipse(String version) {
return new EclipseConfig(version);
}

public class EclipseConfig {
final String version;
Object[] configFiles;

EclipseConfig(String version) {
configFiles = new Object[0];
this.version = Objects.requireNonNull(version);
addStep(createStep());
}

public void configFile(Object... configFiles) {
this.configFiles = requireElementsNonNull(configFiles);
replaceStep(createStep());
}

private FormatterStep createStep() {
Project project = getProject();
return EclipseFormatterStep.create(version,
project.files(configFiles).getFiles(),
GradleProvisioner.fromProject(project));
}
}

/** If the user hasn't specified the files yet, we'll assume he/she means all of the java files. */
@Override
protected void setupTask(SpotlessTask task) {
Expand Down
2 changes: 1 addition & 1 deletion plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SelfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ private static void runTasksManually(Type type) throws Exception {
java.target("**/*.java");
java.licenseHeaderFile("spotless.license.java");
java.importOrderFile("spotless.importorder");
java.eclipseFormatFile("spotless.eclipseformat.xml");
java.eclipse().configFile("spotless.eclipseformat.xml");
java.trimTrailingWhitespace();
java.customLazy("Lambda fix", () -> raw -> {
if (!raw.contains("public class SelfTest ")) {
Expand Down
2 changes: 1 addition & 1 deletion spotlessSelf.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ spotless {
bumpThisNumberIfACustomStepChanges(1)
licenseHeaderFile 'spotless.license'
importOrderFile 'spotless.importorder'
eclipseFormatFile 'spotless.eclipseformat.xml'
eclipse().configFile 'spotless.eclipseformat.xml'
trimTrailingWhitespace()
removeUnusedImports()
}
Expand Down