Slf4j support #342
Slf4j support #342
Conversation
There was a problem hiding this comment.
This looks great! My only requested changes are about documentation. We should keep it package-private to give ourselves as much flexibility as possible.
Also, the only way that lib consumers can access this is through the public API, so we should document this change there.
I would change the wording in JarState.getClassLoader which explains that the slf4j package will be punched through from the host classloader. I would also add a comment at FeatureClassLoader.BUILD_TOOLS_PACKAGES which says "if this ever changes, make sure to update docs at JarState.getClassLoader.
| * the build tool and the provided URLs are ignored. This allows the feature to use | ||
| * distinct functionality of the build tool. | ||
| */ | ||
| public class FeatureClassLoader extends URLClassLoader { |
| @@ -16,12 +16,9 @@ | |||
| package com.diffplug.spotless.extra.eclipse.base.osgi; | |||
|
|
|||
| import org.osgi.framework.Bundle; | |||
| import org.osgi.service.packageadmin.ExportedPackage; | |||
| import org.osgi.service.packageadmin.PackageAdmin; | |||
| import org.osgi.service.packageadmin.RequiredBundle; | |||
There was a problem hiding this comment.
What's up with this change?
There was a problem hiding this comment.
Sorry, been lazy. Changed this because of deprecation warnings. But I looked it up again. Was actually a bug in an Open-JDK version I used on one system. The standard states that the @SuppressWarnings("deprecation") on the outer class should be sufficient. Will undo it.
| //Configure SLF4J-Simple | ||
| System.setProperty(SimpleLogger.LOG_FILE_KEY, "System.out"); | ||
| System.setProperty(SimpleLogger.SHOW_SHORT_LOG_NAME_KEY, "true"); | ||
| System.setProperty(SimpleLogger.DEFAULT_LOG_LEVEL_KEY, "warn"); |
There was a problem hiding this comment.
Do you need to remove these system props after the test is done?
There was a problem hiding this comment.
Since it is the only logging test... and the only test where the Eclipse-SLF4J logging service is temporarily applied... Let's leave it.
| try { | ||
| ClassLoader.registerAsParallelCapable(); | ||
| } catch (NoSuchMethodError ignore) { | ||
| // Not supported on Java 6 |
There was a problem hiding this comment.
Any value in logging that this functionality is disabled because of the version of Java?
There was a problem hiding this comment.
Sorry, the comment is misleading. The feature was introduced with Java 7. Will update the comment.
There was a problem hiding this comment.
Also, Spotless requires Java 8 - heavy usage of lambdas and functional types. I'm fine with this backcompat if you want to keep it, but I'd also be fine with removing it if it's easier.
...-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/service/SingleSlf4JService.java
Show resolved
Hide resolved
| public void log(LogEntry entry) { | ||
| synchronized (listener) { | ||
| listener.stream().forEach(l -> l.logged(entry)); | ||
| } |
There was a problem hiding this comment.
Consider using a ReadWriteLock instead of synchronizing on listener.
If you use a ReadWriteLock you can have multiple threads logging at the same time.
There was a problem hiding this comment.
Principally a good idea, but I think Java 8 does not provide guards for it, or does it?
If not, I find to verbose (guards implementation) or to error prone (without guards).
Eclipse code checks whether debug level is enabled. The formatter anyhow just log warnings. And most of the underlying Eclipse functionality is stubbed by my framework. So this should really not be performance critical.
OK?
There was a problem hiding this comment.
Could it be a bottleneck in a large multi-project build where spotless is running in parallel on a lot of projects?
There was a problem hiding this comment.
Would doubt it. Warnings I saw so far end up in not formatted files (and then due to null-edits in exceptions the Spotless wrapper throw on purpose).
One reason I wanted this logger was that I found bugs in my wrapper implementation quite late and then realized that I just missed a warning.
|
Sorry for slow response @fvgh, I missed the notification about new commits. I just added a small commit tweaking the docs. This LGTM, feel free to merge whenever you'd like. |
|
I've got a few hours to spare, to so I'm gonna take on some of our low-hanging fruit. Please let me know if there's anything you'd like me to publish from |
# Conflicts: # CHANGES.md
The PR provides the Spotless features with access to the SLF4J interface of the underlying build tool.
In case the build tools associated to a Spotless plugin does not provide SLF4J, the Spotless plugin must provide an implementation mapping to the logging feature of the build tool.
The changes fixing #236 and are the result of the discussion and POC provided for that issue.
For the new Eclipse-Base service, UTs have been provided. The Maven and Gradle plugin have been tested with a patched Spotless Eclipse-JDT. Since the changes to the Spotless LIB are quite simple, UTs are currently not foreseen.
@nedtwigg Please provide a new release of Eclipse-Base. It's change log still contains a TDB for the release date.