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

fvgh · 2017-05-05T06:13:37Z

To complete my open task as discussed.
I checked the old eclipseFormatFile manually before I modified spotlessSelf.gradle. Let me know if you prefer an automated test.

… discussed in #94.

jbduncan

Hi @fvgh, just have a couple of comments, otherwise LGTM!

jbduncan · 2017-05-05T06:21:27Z

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

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

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


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

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

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

jbduncan · 2017-05-05T06:26:49Z

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

@@ -59,17 +63,23 @@ public void importOrderFile(Object importOrderFile) {
 		addStep(ImportOrderStep.createFromFile(getProject().file(importOrderFile)));
 	}

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


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

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

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?

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. */

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.

Hmm, @nedtwigg, what do you think?

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

jbduncan · 2017-05-05T06:40:52Z

@fvgh Although, having said in my review that things LGTM, it's not clear to me what you meant when you said,

I checked the old eclipseFormatFile manually before I modified spotlessSelf.gradle. Let me know if you prefer an automated test.

Could you elaborate for me what you meant by checking the old eclipseFormatFile before modifying spotlessSelf.gradle, and what this automated test would catch?

fvgh · 2017-05-05T18:16:41Z

Sorry for being that brief about

checked the old eclipseFormatFile

This is actually really something we need an agreement on, and I should have therefore be more explicit.
I basically refactored all tests to use eclipse().configFile instead of eclipseFormatFile, but eclipseFormatFile is:

still supported and used currently by many projects, so it will annoy a lot of people if it fails
augmented by new functionality, the logging of the warning that the method is deprecated

So all I did for testing is checking before I replaced eclipseFormatFile in spotlessSelf.gradle, whether the build works and I get the expected warning.
Looking at the code changes it can be argued that it is this code change is simple and unlikely to change in future. That's why I am currently did not provide an automated test for the deprecated interface.
So let me know whether this is sufficient for you.

jbduncan · 2017-05-05T18:28:16Z

I admit it's not clear to me how we'd even test that the deprecated interface is no longer used. Mocking? This is an area where I know very little.

One one hand, I see this as a kind of refactoring, so if we don't get deprecation warnings at compile time, I'd be happy, On the other hand, eclipse().configFile() does do logging that eclipseFormatFile() doesn't (as you mentioned @fvgh), and I'm also not clear on what Spotless's deprecation policy is.

@nedtwigg, do you have any thoughts on this?

fvgh · 2017-05-05T18:45:56Z

@jbduncan Sorry, I haven't explained it very well. No mocking is required.
I can add in SelfTest.java a an additional small test for eclipseFormatFile, or you accept my argument and say that manual testing is good enough.
The testing of the new warning takes a few lines more, but I think that goes beyond what you normally test for spotless.

fvgh · 2017-05-05T18:51:21Z

As you can see, eclipseFormatFile now just call eclipse().configFile. So even if (unlikely) somebody fumbles with the underlying code, it is assured that eclipseFormatFile still will work as long as the automated tests for eclipse().configFile still work. And anyhow, if in doubt, just replace in spotlessSelf.gradle the eclipse().configFile by eclipseFormatFile and see what happens.

nedtwigg · 2017-05-05T20:22:00Z

This LGTM. This isn't anyone's fulltime job, so I think it's okay if we don't maintain tests for every deprecated feature. I'll leave it to @jbduncan to press the rebase and merge button if you feel your concerns are addressed well-enough. Totally fine to continue discussion too if they aren't :)

fvgh · 2017-05-05T20:31:58Z

@nedtwigg I am afraid I am already too tiered and should call it a day. I think I missed one of @jbduncan concerns which is the "deprecation policy".
I think the deprecation warning is just considered a friendly reminder that something will/should be removed from the code in the future. So would you agree that principally there is no drawback in having a deprecated warning for the test?

nedtwigg · 2017-05-05T20:35:46Z

So would you agree that principally there is no drawback in having a deprecated warning for the test?

The upside - the deprecated functionality is being tested. The downside - there's a deprecated warning on compile. I see this as 50/50, implementation wins.

jbduncan · 2017-05-05T20:36:36Z

I'm overall rather happy with this PR! As-is, I'd be happy to merge it in. My only concern left is whether the usage of @Deprecated on eclipseFormatFile() follows Spotless's deprecation policy, but if it needs to be changed, I'm sure we can address that in a future commit.

@fvgh I think there's no need for an automated test for checking eclipseFormatFile()-to-eclipse().configFile() - I think your manual check is good enough!

So I'll merge this in now. :)

nedtwigg · 2017-05-05T20:53:14Z

whether the usage of @deprecated on eclipseFormatFile() follows Spotless's deprecation policy

I think the standard industry policy is to mark deprecated methods as @deprecated, document the new usage, and emit a warning of some kind to prompt the user to update / maintain their usage. I think this PR checks all those boxes.

Might be ways to improve on this though. Suggestions?

jbduncan · 2017-05-05T20:58:10Z

...I think this PR checks all those boxes.

Sounds good to me. 👍

Suggestions?

I can only think of Guava's deprecation policy, but I don't think it's so applicable to Spotless because it involves a mixture of a custom @Beta annotation and waiting for a few months or one or two releases before removing a deprecated feature - probably too heavyweight for us.

Replaced importOrder array argument by varargs. Fixes #125.

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

1cd6bbe

… discussed in #94.

jbduncan approved these changes May 5, 2017

View reviewed changes

jbduncan merged commit 7aded96 into master May 5, 2017

jbduncan deleted the align_eclipse_formatter_config_for_gradle branch May 5, 2017 20:38

fvgh referenced this pull request Jul 28, 2017

Merge pull request #126 from diffplug/importOrderVarargs

0b5fe2d

Replaced importOrder array argument by varargs. Fixes #125.

This was referenced Jul 28, 2017

Task spotlessJava not triggering when only formatting code #127

Closed

Added missing user warnings for deprecated methods. #128

Merged

fvgh mentioned this pull request Sep 20, 2017

Add support for ktlint to respect .editorconfig #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fvgh commented May 5, 2017

jbduncan left a comment

jbduncan May 5, 2017

nedtwigg May 5, 2017

jbduncan May 5, 2017

fvgh May 5, 2017

jbduncan May 5, 2017 •

edited

Loading

fvgh May 5, 2017

jbduncan May 5, 2017

nedtwigg May 5, 2017

jbduncan commented May 5, 2017

fvgh commented May 5, 2017 •

edited

Loading

jbduncan commented May 5, 2017

fvgh commented May 5, 2017

fvgh commented May 5, 2017

nedtwigg commented May 5, 2017

fvgh commented May 5, 2017

nedtwigg commented May 5, 2017

jbduncan commented May 5, 2017

nedtwigg commented May 5, 2017

jbduncan commented May 5, 2017

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

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

Conversation

fvgh commented May 5, 2017

jbduncan left a comment

Choose a reason for hiding this comment

jbduncan May 5, 2017

Choose a reason for hiding this comment

nedtwigg May 5, 2017

Choose a reason for hiding this comment

jbduncan May 5, 2017

Choose a reason for hiding this comment

fvgh May 5, 2017

Choose a reason for hiding this comment

jbduncan May 5, 2017 • edited Loading

Choose a reason for hiding this comment

fvgh May 5, 2017

Choose a reason for hiding this comment

jbduncan May 5, 2017

Choose a reason for hiding this comment

nedtwigg May 5, 2017

Choose a reason for hiding this comment

jbduncan commented May 5, 2017

fvgh commented May 5, 2017 • edited Loading

jbduncan commented May 5, 2017

fvgh commented May 5, 2017

fvgh commented May 5, 2017

nedtwigg commented May 5, 2017

fvgh commented May 5, 2017

nedtwigg commented May 5, 2017

jbduncan commented May 5, 2017

nedtwigg commented May 5, 2017

jbduncan commented May 5, 2017

jbduncan May 5, 2017 •

edited

Loading

fvgh commented May 5, 2017 •

edited

Loading