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.

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

Common Spotless Eclipse Framework which can be used by most Spotless … #234

Merged
merged 12 commits into from
Jul 15, 2018

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Apr 13, 2018

The Spotless Eclipse framework avoids redundancies during the development of Spotless Eclipse based formatters.
It is a spin-of from #230 and tested successfully to provide solutions for #191 and #226.

Since it is a (small) framework I consider its code-quality more important than for other _ext projects.
I added therefore more checks, but do welcome any proposals for enhancements.
Furthermore, I consider it very important that people understand its usage. I provided a more verbose README.md. Please let me know what I can enhance in terms of language but also if you have proposals for the interface.

@fvgh fvgh requested a review from nedtwigg April 13, 2018 18:13
@fvgh
Copy link
Member Author

fvgh commented Apr 13, 2018

@jbduncan Don't know whether you have time, but I always appreciated your comments. Maybe you can have also a look at this. THX

@fvgh
Copy link
Member Author

fvgh commented Apr 15, 2018

One remark: Just noticed that the package name is com.diffplug.gradle.spotless.eclipse.
This is just a heritage of the eclipse-jdt.
Since neither Spotless nor these formatters are restricted to gradle, I would propose a refactoring.
For example com.diffplug.gradle.spotless.java.eclipse should be refactored to com.diffplug.spotless.extra.java.eclipse. This would emphasize that the code is developed for com.diffplug.spotless.extra, but still follows the convention <spotless>.<language>.<formatter>.
Shall we take the opportunity for a refactoring of this and the other eclipse based extensions?

Eclipse core uses plugins not derived from Plugin but BundleActivator.
Provided possibility to configure global preferences.
Usage of Consumer allows the user to get rid of statics.
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Looks great! I only found two very minor things.

ext_VER_JAVA=1.8

# Compile dependencies
VER_ECLIPSE_CORE_RESOURCES=[3.11.0,4.0.0[
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be 4.0.0]?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, Eclipse is quite clear that major version changes mean always an incompatibility in interfaces. So I don't expect to support 4.X.
But as I stated in #226, this range is anyhow guess work, since I am not able to use only API calls.
In my opinion, the lib-extra should tie down tested versions, but also give the user the possibility to override these versions (for early adopters).

}
BundleFile bundleFile = null;
try {
bundleFile = jarOrDirectory.isDirectory() ? new DirBundleFile(jarOrDirectory, false) : new ZipBundleFile(jarOrDirectory, null, null, null);
Copy link
Member

Choose a reason for hiding this comment

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

maybe just return jarOrDirectory...? Saves 2 lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean here? I need the BundleFile for the Eclipse interfaces.
And the exception handling/reduction I did not want to do within the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

Option A:

BundleFile bundleFile = null;
try {
	bundleFile = jarOrDirectory.isDirectory() ? new DirBundleFile(jarOrDirectory, false) : new ZipBundleFile(jarOrDirectory, null, null, null);
} catch (IOException e) {
	throw new BundleException(String.format("Cannot access bundle at '%s'.", jarOrDirectory), BundleException.READ_ERROR);
}
return bundleFile;

Option B:

try {
	return jarOrDirectory.isDirectory() ? new DirBundleFile(jarOrDirectory, false) : new ZipBundleFile(jarOrDirectory, null, null, null);
} catch (IOException e) {
	throw new BundleException(String.format("Cannot access bundle at '%s'.", jarOrDirectory), BundleException.READ_ERROR);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I was blind. Thanks for the hint.

@nedtwigg
Copy link
Member

Refactoring package names of anything in _ext is fine by me. Overall this LGTM, merge at will :)

@fvgh
Copy link
Member Author

fvgh commented Apr 27, 2018

Thank @nedtwigg
As I stated in #226, I would like the change the package names from com.diffplug.gradle.spotless.eclipse to com.diffplug.spotless.extra.eclipse.
Nearly finished my work on JDT and WTP. Found no more reasons to change the interface, except #236. But I will leave this out of the PR.

@jbduncan
Copy link
Member

jbduncan commented Apr 27, 2018

Hi @fvgh, very sorry for not responding sooner!

I've been busy with all sorts of things over the last few weeks, so I've struggled to find the time to properly look at this PR.

I did have a quick look some time ago, but I'm actually not sure what I could review other than the small stuff (e.g. using Array(List|Deque) instead of LinkedList) as well as the formatting, as I know next to nothing about Eclipse's internals. However, I understand that reviewing the formatting wouldn't be a good use of my time, as I recall that @nedtwigg is perfectly happy for spotlessCheck to determine what counts as good formatting or not.

@fvgh @nedtwigg How would you feel if I were to just go over any small non-formatting things that seem out-of-place to me?

@jbduncan
Copy link
Member

@fvgh Alternatively, did you have any ideas as to areas of the code that you wanted feedback on?

@fvgh
Copy link
Member Author

fvgh commented Apr 27, 2018

@jbduncan
First of all it would be nice if the README.md is more or less understandable.

I am also not sure whether the interface solution for the SpotlessEclipseFramework could be better.
I used the enums so that I have a hopefully reasonable documentation of the settings.
The consumers I added to avoid the user to use statics to keep track whether the framework has already been setup. Currently we always use anyhow separate class loader for each external formatter step instance, but I think this might change in future. And the statics are also ugly during testing. Maybe you have any ideas, but I see that this is maybe hard to tell without proper examples how the SpotlessEclipseFramework is used.

Beside the SpotlessEclipseFramework, I think the only important classes are the BundleController, PluginRegistrar, BundleSet and ServiceCollection. The other once are just stubbing existing interfaces.

If you find time, that would be great.

This improves the development of Eclipse formatters.
@fvgh
Copy link
Member Author

fvgh commented Apr 29, 2018

@jbduncan
#241 provides some examples how I am using the SpotlessEclipseFramework interface in a more 'difficult' scenario.
#239 shows what I think will be the nominal use case of SpotlessEclipseFramework.

If you have any proposals how to enhance/replace the SpotlessEclipseFramework.setup, let me know.

@nedtwigg
Copy link
Member

nedtwigg commented May 2, 2018

Just FYI @fvgh, from the time you say "I'm ready, publish these artifacts", it will take up to 48 hrs before the first version is in mavencentral (have to do a manual process with bintray to publish a new artifact). From there we can publish new versions ~instantly. I'm ready when you are, but in no hurry. All your PR's look good. When you want to publish, just

  • press merge
  • tell me you want to publish, and I'll pull the trigger

I'm happy to do them all at once, or if you wanna do some infrastructure piece first, then something language-specific, whatever. I don't think back-compat in these _ext libraries is a big deal, we can bump the major version at will, so it's not a huge deal to get the API exactly right for the first publish.

@fvgh
Copy link
Member Author

fvgh commented May 2, 2018 via email

@fvgh
Copy link
Member Author

fvgh commented May 2, 2018 via email

@jbduncan
Copy link
Member

jbduncan commented May 2, 2018

@fvgh

If you have any proposals how to enhance/replace the SpotlessEclipseFramework.setup, let me know.

I don't have any better ideas that what you already have, so SpotlessEclipseFramework LGTM!

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

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

@fvgh I found a few things with the documentation and one line of code that I thought could be improved.

Other than that, I'm impressed by the apparent time and effort that went into this, as well as the knowledge that must have been needed to realise how to mock Eclipse's internals, so bravo!

(I admit that I've still ignored the Eclipse-y, high-level parts, for my knowledge of Eclipse's internals are minimal, to say the least, and so I don't know how to review them. But other than that and the points I address below, looks good to me.)

@@ -0,0 +1,66 @@
# spotless-eclipse-base

Eclipse formatters are part of the the Eclipse User Interface implementations. Hence their public interfaces are depending on various Eclipse modules.
Copy link
Member

Choose a reason for hiding this comment

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

Am I right to understand from this paragraph that, the way Eclipse works internally, formatters depend on details from the user interface part(s) of Eclipse itself?

If yes, then perhaps this paragraph would read better as something like this?

Eclipse formatters are intrinsically linked to the user interface of Eclipse proper. As a consequence, the formatters' public interfaces depend on various, seemingly-unrelated Eclipse modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid the GUI is not the problem. I've rewritten it as follows:

Eclipse formatters are embedded in plugins serving multiple purposes and not necessarily supporting headless builds. Hence the plugin interfaces are depending on various Eclipse plugins and services not required for the formatting purpose.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM. Ta. :)


Eclipse formatters are part of the the Eclipse User Interface implementations. Hence their public interfaces are depending on various Eclipse modules.

Spotless provides its own plugin framework with `com.diffplug.spotless.JarState`. This allows Spotless the usage of different Eclipse versions in parallel.
Copy link
Member

Choose a reason for hiding this comment

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

s/the usage of/to use/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Spotless provides its own plugin framework with `com.diffplug.spotless.JarState`. This allows Spotless the usage of different Eclipse versions in parallel.


The `com.diffplug.gradle.spotless:spotless-eclipse-base` artifact mocks the redundant Eclipse OSGI/plugin framework for Spotless. Furthermore it provides Eclipse services adapted for Spotless, which avoids for example the creation of a permanent workspace and reduces the dependencies on Eclipse modules unused by the Eclipse code formatters.
Copy link
Member

Choose a reason for hiding this comment

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

s/Spotless/Spotless's own use/ ?

Copy link
Member

Choose a reason for hiding this comment

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

s/workspace/Eclipse workspace/ ?

I think this makes it more explicit that "workspace" refers to an Eclipse workspace in this context rather than something else.

Copy link
Member

Choose a reason for hiding this comment

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

s/the dependencies/dependencies/ ?


## Usage

Include the artifact to your Spotless Eclipse formatter project, whereas the major version must match the Eclipse core version your formatter supports. The exact default version should be selected by the **lib-extra**.
Copy link
Member

Choose a reason for hiding this comment

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

s/to/in/ ?

Copy link
Member

Choose a reason for hiding this comment

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

s/whereas/where/ ?

}
```

In the constructor of your formatter, the Spotless Eclipse Framework can be configured depending on your formatters requirements. For example the JDT formatter can be configured like:
Copy link
Member

Choose a reason for hiding this comment

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

s/formatters/formatter's/ ?

/**
* Default plugins required by most Spotless formatters.
* <p>
* Eclipse plugins are OSGI bundles itself and do not necessarily derive from the Eclipse Plugin class.
Copy link
Member

Choose a reason for hiding this comment

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

s/itself/themselves/ ?

* Default plugins required by most Spotless formatters.
* <p>
* Eclipse plugins are OSGI bundles itself and do not necessarily derive from the Eclipse Plugin class.
* BundleActivator implementation may as well server as plugins.
Copy link
Member

Choose a reason for hiding this comment

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

s/BundleActivator/{@code BundleActivator}/ ?

Copy link
Member

Choose a reason for hiding this comment

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

s/server/serve/ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

👎 I do not agree to the {@code ... usage.
In my opinion the JavaDoc does not profit a lot by the usage and I always find it distracting and too verbose when I study code. The IDE anyway supports the linkage to the class sources with or without it.

Copy link
Member

Choose a reason for hiding this comment

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

👎 I do not agree to the {@code ... usage.
In my opinion the JavaDoc does not profit a lot by the usage and I always find it distracting and too verbose when I study code. The IDE anyway supports the linkage to the class sources with or without it.

Ah okay. I'm personally a fan of it, but I don't want to push things if @nedtwigg isn't a fan of it either.

@nedtwigg Do you have a preference regarding this?

Copy link
Member

Choose a reason for hiding this comment

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

My preference here goes to the author. I don't think it's super-important for this to be perfectly consistent across the codebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I had a look at the other Spotless code.
I will adapt mine to be in line with the usage of @link / @code.

@nedtwigg
Copy link
Member

nedtwigg commented May 2, 2018

I tested everything thoroughly, but I would appreciate if you could publish everything as a snapshot version until I finalized the integration. Despite the things that are common for Gradle and Maven, I will for now only the integration into Grade. OK for you?

Sounds great. FWIW, I've had better luck using mavenLocal and publishToMavenLocal for integration testing than anything else.


/** Sets property/preference values available to all bundles, plugins and services. */
default void set(Map<String, String> properties) {
properties.entrySet().stream().forEach(x -> set(x.getKey(), x.getValue()));
Copy link
Member

Choose a reason for hiding this comment

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

Oh, somehow forgot to include this in my last review!

I believe that this can be shortened to:

properties.forEach(SpotlessEclipseServiceConfig::set);

Copy link
Member

@jbduncan jbduncan May 2, 2018

Choose a reason for hiding this comment

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

I think you're right in that in this case, rather than using SpotlessEclipseServiceConfig::set, the thing to use would be this::set, as we're referring to an instance method rather than a static method a method of this rather than of another class or instance.

However, by my understanding (I haven't checked this in an IDE), it should be possible to write just this:

properties.forEach(this::set);

as the signature of this::set matches Map::forEach.

But if that doesn't compile, then the following should work perfectly fine too:

properties.forEach((k, v) -> set(k, v));

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jbduncan for all your effort. I highly appreciate it. I know that it is not the most interesting thing to do. There were a few things I must admit, I should have noticed when I read my own comments. But some of the mistakes I made, I made already for many years. And since nobody told me that it's wrong...

Yes, I already made further changes, but not related to the issues you found. Most of it, was related to the 'miss-usage' of BundleController.SystemBundle, which was no longer a system bundle how OSGi defines it. And I needed an extension/configuration to run different bundles in different states, depending who asks.
My changes, probably not flawless, improved the readability code, so I think its OK to commit them without further review. Thanks again for your work.

fvgh added 2 commits May 5, 2018 11:50
Enhanced BundleController by creating separate class for SimpleBundel (previously called SystemBundle).
Added support for plugins offering headless builds.
@fvgh
Copy link
Member Author

fvgh commented May 5, 2018

@nedtwigg Can you have a brief check, whether my changes to the snapshpot publication are alright with you?
I will then test the Spotless integration of the Eclipse formatters before merging the formatter implementation branches.

@nedtwigg
Copy link
Member

nedtwigg commented May 5, 2018

Looks great! Definite improvement on the old system.

@nedtwigg
Copy link
Member

Hi @fvgh - just wanted to check that you're not blocked on me :)

@fvgh
Copy link
Member Author

fvgh commented May 24, 2018

@nedtwigg Sorry, I have not much time. Just preparing the underlying changes for the EclipseFormatterStep and GrEclipseFormatterStep to support multiple versions, changes and maintain their dependencies. Hope to finalize it by end of the month.

@nedtwigg
Copy link
Member

No time pressure from me!!! Just wanted to double-check that I wasn't holding anything up.

try {
return jarOrDirectory.isDirectory() ? new DirBundleFile(jarOrDirectory, false) : new ZipBundleFile(jarOrDirectory, null, null, null);
} catch (IOException e) {
throw new BundleException(String.format("Cannot access bundle at '%s'.", jarOrDirectory), BundleException.READ_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

You loose the original stack trace here. You should pass the causal exception into this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JLLeitschuh Thanks a lot.
Do you know whether I can check automatically for such an "error". It is not always, but in 95% of the cases, an error. So I was wandering whether FindBugs or something similar can do something for me, where I just exclude the 5% of the correct cases by an annotation.

Copy link
Member

Choose a reason for hiding this comment

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

@fvgh I think Google's error-prone project catches this sort of error now, but I don't think the corresponding check has been released in a versioned release of error-prone yet.

Applied changes requested by review (@JLLeitschuh).
@fvgh
Copy link
Member Author

fvgh commented Jun 4, 2018

@JLLeitschuh No, sorry, should have split the commit. @nedtwigg came up with the great idea to use the latest Gradle feature available in their next release. I still needed something which provides me with the coordinates used during the build and coincidentally Gradle will provide this feature in its next release with nearly the same file format I already implemented for #253 .
Sorry, had not much time lately and got a little bit hectic getting the implementation done.

@nedtwigg I put a version check around it:

        if (gradleVersion >= requiredVersion) {
            resolutionStrategy.activateDependencyLocking()
        }

Since it is not required for the actual build, but just to generate input for lib-extra, the current Gradle version used by Spotless still works for building and publishing. Once there is a Gradle release we can update that version in another PR.

@fvgh
Copy link
Member Author

fvgh commented Jul 14, 2018

@nedtwigg Please have a last look on bed43ed. I removed the model space from the publisher since it prevented publishToMavenLocal for releases. To be frank, I did not really understood why you inserted it in the first place...
I am ready to do a squash merge. Don't know though, why my last commits (like the one I referenced above) are not showing up in this PR.

@nedtwigg
Copy link
Member

LGTM! I've always struggled with gradle and maven publishing, glad to get rid of model if we don't need it 👍

@fvgh fvgh merged commit 263a03f into master Jul 15, 2018
@nedtwigg
Copy link
Member

nedtwigg commented Jul 15, 2018 via email

fvgh added a commit that referenced this pull request Jul 18, 2018
Applied common Spotless Eclipse framework to JDT (see #234).
Updated version and artifact ID as discussed in #226.
nedtwigg pushed a commit that referenced this pull request Jul 18, 2018
* Common Spotless Eclipse Framework which can be used by most Spotless Eclipse based formatters.

* Added usage information for fat JARs.

* Make developers in common java-publish.gradle configurable.

* Enhanced framework interfaces.
Eclipse core uses plugins not derived from Plugin but BundleActivator.
Provided possibility to configure global preferences.
Usage of Consumer allows the user to get rid of statics.

* Refactored directory structure as discussed in #234

* Refactored directory structure as discussed in #234

* Adpated package names according to previous commit. Applied fixes as requested in #234. Refactored artifact name and version as discussed in #226.

* Moved LINE_DELIMITER to SpotlessEclipseFramework

* Enhanced error report in case OSGI bundle creation fails.
This improves the development of Eclipse formatters.

* Applied review proposals.
Enhanced BundleController by creating separate class for SimpleBundel (previously called SystemBundle).
Added support for plugins offering headless builds.

* Allow configuration of snapshot repository for upload.

* Added Gradle dependency lock support.
Applied changes requested by review (@JLLeitschuh).

* Changed "unchecked" warning suppression for Java 10 compliance.

* Replaced JavaDoc @see with @code to avoid warnings when referring to implementations in dependencies.

* Added missing method implementation (org.eclipse.osgi update from 3.12 to 3.13)

* Fixed bintray usage. Plugin-ID for bintray does not work within script (see #1262).
Removed model space to allow local publishing of releases.

* Finalized version 3.0.0
@fvgh fvgh mentioned this pull request Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants