John Murph wrote:
I have some code for this at git://github.com/sappling/gradle.git
<http://github.com/sappling/gradle.git> in branch listener_manager.
I'm not sure if we should integrate this as is, or finish changing
over all the other existing listeners to this scheme first. Next week
I will try to get a TestListener submitted that can listen to JUnit
(and testng, hopefully) tests. For that purpose, this code also
contains generic listener remoting support in package
org.gradle.listener.remote.
Please take a look at it give me some feedback. I'm especially
concerned if the remoting support seems like the right approach (and
suggestions for error handling in this code would be nice too).
Some comments:
- Generally, I think this is the right direction to take. However, I
think it's a little invasive at the moment. More on this below.
- I don't think we should use the Listener marker interface. Requiring
it means, for example, we can't use 3rd party listener interfaces. It's
just unnecessary coupling. We should be able to broadcast on any
interface which has only void methods.
- We should leave the strongly typed addNnnListener() methods, rather
than replace them with a single addListener(). We should add
addListener() as a convenience only.
Having a single method means we lose the static typing and documentation
goodness. It also means we can't have different event streams which use
the same interface, such as, GradleLauncher.addStandardOutputListener()
and addStandardErrorListener().
I think the only classes which should have an addListener() method are
Gradle and GradleLauncher. These are the only places where adding a
global listener of a given type make sense.
- I think the changes to LoggingConfigurer are a good example of how the
approach is a little too invasive. To me, ListenerManager and
LoggingConfigurer are services, and it's always a smell (to me) when a
service is passed into another service via the service interface
methods. Why should I need a ListenerManager to configure logging? It
feels like ListenerManager is leaking into places it doesn't really belong.
Some alternative approaches:
1. Inject the ListenerManager into the DefaultLoggingConfigurer, and
leave the addNnnListener() methods on LoggingConfigurer.
DefaultLoggingConfigurer uses the ListenerManager to create 2
broadcasters with type StandardOutputListener. That is, it creates 2
event streams. One stream is for standard out, the other for standard error.
2. Move the above logic out of DefaultLoggingConfigurer, and inject the
2 broadcasters into DefaultLoggingConfigurer. Remove the
addNnnListener() methods. This way, DefaultLoggingConfigurer know
nothing of ListenerManagers. It just knows it needs to emit std out and
std err streams.
interface LoggingConfigurer {
void configure(LogLevel level)
}
class DefaultLoggingConfigurer {
public DefaultLoggingConfigurer(StandardOutputListener
stdoutBroadcaster, StandardOutputListener stderrBroadcaster) {
...
}
public void configure(LogLevel level) {
... attaches the broadcasters to the appropriate appenders ...
}
}
I think I like option 2.
- DefaultInternalRepository should not register itself. In general, I
think it's better for a service to simply declare that it is a listener
of some type and have the environment take care of the registering,
rather than for the service to know how to register itself. This is just
a variation of inversion of control.
- Remote approach looks good, but I would hold off going too far down
that path until the native test changes are merged in, because it does
pretty much the same stuff.
Adam
