John Murph wrote:
On Mon, Sep 14, 2009 at 2:11 AM, Adam Murdoch <[email protected]
<mailto:[email protected]>> wrote:
John Murph wrote:
I have my latest iteration up on GitHub (at
[email protected]:jmurph/gradle.git branch listener_manager). I
(think I) did everything you requested except
1) I left the StandardOutputListener and StandardErrorListener
stuff combined into a single StandardOutputListener because it
just seems like those are really one concept and having a
single listener made more sense to me.
There's actually 2 concepts here:
- The type of event being received. In this case, we're receiving
log message events.
- The source of the events. In this case, there's 2 sources,
namely stdout log messages and stderr log messages.
The problem with using a single interface for these 2 concepts is
that you statically bind the concepts together. This forces every
listener implementation to deal with the source, even if when
doesn't care. For example, a logging listener which just appends
all logging messages to a text panel must deal with both stdout
and stderr events, even though it does not care about the
distinction. This makes it more difficult to write reusable,
composable listener implementations that you can wire together
(dynamically) in different ways.
Another problem with static sources in the listener interface is
that you can't change the set of sources without changing the
interface. Or introducing another, very similar, interface. The
set of sources we care about are quite likely to change. For
example, we might want to add some way to receive all the logging
events generated by Ant, regardless of whether they end up on
stdout or stderr. Or perhaps all logging events generated at info
log level. Or perhaps the stderr logging events generated by the
build script.
As a general pattern, I think we should provide small listener
interfaces with maybe 1 method, or possibly 2 methods for a
started/completed type event stream. The interfaces should not
encode the source in the names of the methods (as a parameter of
the method is fine). A listener can pick and choose which of these
interfaces it wants to implement. This makes us resilient to
change, and allows listeners to be composed from reusable pieces.
I think that's generally a good rule, but rules have exceptions for a
reason, and I think this is one.
We didn't need the exception before we started adding the
ListenerManager. We started from having the logging event API follow the
rule. ListenerManager is a piece of infrastructure, and shouldn't really
have such an impact on the public API. The fact that it has suggests to
me that we've not got the infrastructure quite right.
If stdout and stderr were really different sources, I would agree
with you. Since they are two sides of the same coin, and always
paired together your argument seems overly academic.
But they're not always paired together from the consumer's point of view
(that is, the user of these APIs).
Sometimes I want to do exactly the same thing for all log messages
regardless of where they come from. For example, I might want to append
each log message to the end of a text pane in a UI. In this case, I
don't care about the distinction between stdout and stderr. I don't want
the listener interface to make me do something for stdout and something
for stderr.
Sometimes I want to do something with the stderr messages only. For
example, I might gather the error log messages up into an email to send
at the end of the build. In this case, I don't care about stdout. I
don't want the listener interface to make me do something for stdout and
something for stderr.
In fact, you can reduce all usages of logging events to combinations of
doing the same stuff for all log messages, and doing stuff for just one
source. And neither of these cases are well served by a listener
interface that has a method for stdout and a method for stderr.
We didn't have these problems with a single method listener interface.
But really, I would change it except I'm not sure how.
The design of ListenerManager is that listeners are defined by their
type, not by usage. That's because in most cases the two are the
same, and I didn't like the redundancy. So, I have a couple of
options for splitting these streams back apart but I don't like any of
them.
1) Rename the original StandardOutputListener to
StandardStreamListener. Make two subtypes StandardOutputListener and
StandardErrorListener that extend StandardStreamListener by are
empty. These would be marker interfaces just to allow ListenerManager
and friends to talk about type instead of usage.
2) Change the original StandardOutputListener's onOutput method to
take a source parameter (of type String, or some stronger type?).
This would allow one interface to be used for multiple purposes, but
causes listeners to get feedback for all sources, even ones they might
now know about (such as Ant events). This is probably the smallest
change.
3) Change ListenerManager to be are of usage. So, a listener would be
registered along with a (optional?) usage identifier. When a
broadcaster is requested, the type of the listener and it's intended
usage would have to be given. This complicates ListenerManager and
it's usage but supports these kinds of situations in the future
without requiring that 1) or 2) be done again for each case.
I suggest we:
- Add methods to the ListenerManager interface so that clients can
create anonymous ListenerBroadcaster instances of a given type. Each
would be equivalent to a source, but its really up to the client as to
what the meaning is.
- Change the implementation of this to also create a global
ListenerBroadcaster of the requested type, which receives all events
from all anonymous ListenerBroadcaster instances of the same type.
- Inject a ListenerManager into DefaultLoggingConfigurer. It will use
the ListenerManager to create 2 anonymous ListenerBroadcaster instances,
one for stdout and one for stderr. We keep the
LoggingConfigurer.addNnnListener() methods.
- GradleLauncher.addStdNnnListener() delegates to the LoggingConfigurer,
which attaches the listener to the appropriate ListenerBroadcaster.
- GradleLauncher.addListener(someStdOutListener) delegates to the
ListenerManager, which attaches the listener to the global
ListenerBroadcaster.
Adam
