On Mon, Sep 7, 2009 at 6:55 PM, Adam Murdoch <[email protected]> wrote:
>
> 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.
>
I thought about this as I did it. I like the stronger typing provided by
the Listener interface (although I was uncomfortable with it being a marker
interface, that seemed a bad smell). I would think that Gradle would always
want to wrap 3rd party listener interfaces just to isolate itself from
changes to those third party libraries, so that didn't seem to be a very
important limitation. (Notice that remoting support doesn't require the
Listener interface as it seemed like it would want to use 3rd party
interfaces.) I'm not adamant about this, however, so I'll remove it.
>
> - We should leave the strongly typed addNnnListener() methods, rather than
> replace them with a single addListener(). We should add addListener() as a
> convenience only.
>
I don't like two ways of doing the same thing. Why do we need
addNnnListener() methods and a addListener() method on these classes? That
seems unnecessary and confusing.
>
> 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().
>
That's one reason for the Listener common base class: it povides static
typing and documentation without requiring a rippling of new methods every
time a new listener is defined. It does limit having different even streams
that use the same interface, but I don't think that's generally a good idea
anyway. How often does one wish to listen to standard output and not
standard error (or vice versa)? I think a single interface for listening to
output makes sense, after all one can implement one of the methods as a "do
nothing" just as in every other listener interface which has more than one
method.
> 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 didn't feel compelled to put an addListener() anywhere else, so that seems
reasonable.
>
> - 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.
>
I agree, I think option 2 is better. I didn't think enough about that as I
was doing it, and it did feel somewhat wrong to me. Consider it done.
>
>
> - 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.
>
I disagree. DefaultInternalRepository requires that it be registered as a
listener for it to correctly function. Exposing it's implementation details
to the environment seems wrong. I would rather it know how to register
itself. I don't see this as an inversion of control issue as the repository
class is not finding the listener manager with which it should register,
that would be an inversion of control issue. It's being given the listener
manager. Perhaps you would rather that is was given a ListenerBroadcaster
object with which it can register? That way it still makes clear that it
must be registered as a listener in order to function correctly.
> - 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.
>
I didn't know the native test changes did this, that's cool. I went down
this road because we need this functionality to implement the TeamCity
plugin stuff, so I did it. I'll be curious to see what Tom has done here as
my current implementation seems very light weight.
>
> Adam
>
>
--
John Murph
Automated Logic Research Team
