commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <>
Subject Re: [digester] observations on refactored plugins
Date Tue, 06 Apr 2004 22:38:41 GMT
Hi Robert,

Thanks for taking the time to look at this.

On Wed, 2004-04-07 at 08:39, robert burrell donkin wrote:
> a few disconnected and unordered observations about and comments on the 
> refactored plugins package:
> 1 it would be good to have a package html describing the strategies 
> package.

consider it done.

> 2 consider implementing the logical interfaces using abstract classes 
> for RuleFinder and RuleLoader. it is unlikely that implementations will 
> every need to extend anything else and the interfaces may possibly need 
> to be varied later whilst retaining backwards compatibility. (for 
> example, you may find it necessary to pass in a context object.)

Ok, will do.

> 3 i like the package documentation for plugins.

> 4 consider whether you could factor the code in LogUtils in a different 
> way so that it would be possible for users to use a logging strategy 
> plugin. it might possible now to find a home for this method and add a 
> parameter or two whilst retaining the usual default behaviour.

I'd prefer not to go there for the moment. We had a big discussion on
logging in digester many months ago, and never came to a conclusion that
satisfied everyone. 

The LogUtils class is simply there to solve the issue that with the
current approach to logging used in Digester (which I think is broken),
you simply cannot log if a Digester instance isn't yet available. Rather
than clutter the plugins code with lots of "if (digester == null)"
stuff, I moved it to the trivial LogUtils class.

I've got this logging issue on my list for tackling during 1.7
development. Is it ok with you if we leave this issue until then? 

I should probably put a comment on the LogUtils class saying that it is
only a temporary solution, and no user classes should depend on it. I
should also make it package-scope, not public. User code never *needs*
to use it.

> 5 consider whether to remove the resources parameter from:
> the public PluginManager(PerDigesterResources r, PluginManager parent)
> (since the resources should probably be from the manager). it's easy to 
> add new constructs to an API if there is a demand.

I'll think about this.

> 6 in some ways, PerDigesterResources is a context object. consider 
> whether (in the future) it might be useful to add other methods which 
> might not fit so well with the name. a possible candidate for owning 
> the logging strategy.

Ok, I'll mull this over. I agree that if we put more "active" methods on
this class, rather than mostly get/set methods then "..Resources" might
no longer be appropriate. I am quite fond of the "PerDigester" bit,
though, to make clear that a single instance is shared among all the
PluginRules and PluginManager instances.

> 7 one thing that i have learnt over the years is that good naming is 
> very important when creating a component API. an API that is shipped 
> with a jakarta product may be in use for many years. a good name often 
> indicates that the API is well thought out. a good API is also as small 
> as possible (but no smaller ;) since it is easy to add new methods but 
> hard to correct those mistakenly added. consider whether the class 
> names reflect the roles they play and that each can be justified.


Do you intend to do more investigation of the plugins refactored code?
If not, are you happy for the branch to be merged into the trunk? As you
can see, it doesn't actually touch anything except plugins itself, so
there is no danger of breakage of anything in Digester core.



To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message