commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From robert burrell donkin <robertburrelldon...@blueyonder.co.uk>
Subject Re: [digester] plugins module ready for evaluation
Date Sat, 04 Oct 2003 12:43:04 GMT
hi simon

i've committed your contribution (with a few changes). many thanks. would 
you like to write up something more detailed for the apache newsletter 
(ASAP)? (you'll find this on the wiki 
http://nagoya.apache.org/wiki/apachewiki.cgi?ApacheNewsletterDrafts/Issue2)
?

i have found a few issues that (whilst they did not stop the code being 
committed) i think should be resolved before we could think about 
releasing the plugins code. i'd like to encourage people to comment :)


1. code formatting

i've left simon's field naming convention alone for the time being since 
it's not harmful. i'd prefer all of the code to follow the same pattern 
and so would probably like to change it but i've committed it as-is to 
give people a chance to take a look and offer an opinion.

2. Re: PluginRules.add(String pattern, Rule rule)

             try {
                 ((InitializableRule)rule).postRegisterInit(pattern);
             } catch (PluginConfigurationException e) {
                 // Currently, Digester doesn't handle exceptions well
                 // from the add method. The workaround is for the
                 // initialisable rule to remember that its initialisation
                 // failed, and to throw the exception when begin is
                 // called for the first time.
                 log.debug("Rule initialisation failed", e);
                 // throw e; -- alas, can't do this
                 return;
             }

exception handling tends to be a difficult issue for reusable components. 
digester tends not to throw generic catchable exceptions. (the issue of 
exception handling has been debated ad nauseum on the commons list many 
times in many contexts.) IMHO the arguments in favour of adding a check 
exception to Rules.add are not strong enough to consider breaking 
backwards compatibility.

there are two reasonable approaches that plug-ins could take. the first 
(which is what the current code does) is to silently fail. IMHO this is 
probably the wrong behaviour in this case. why? well, it seems to me that 
a typical reason that the InitializableRule may throw an exception is that 
it is not able to function. this suggest to me that really in this case it 
is not unreasonable to assume that running any digestion from the current 
ruleset may well be caused to fail. it will be more difficult to recover 
and diagnose a problem at this time.

the other approach is to throw a runtime exception subclass. i'd probably 
support this approach.

2. compatibility with other kinds of Rules

i'm not totally convinced that PluginRules will work with custom rules 
(but then again, i might be wrong). maybe this would be worth testing.

3. Exception handling

PluginAssertionError extends Error. i'm not very happy about this. IMHO a 
well behaved component should not throw any Error subclasses. i'd be 
happier for PluginAssertionError to be renamed PluginAssertionException 
and extend RuntimeException.

i'm also a bit confused why PluginDeclarationRule throws 
ClassNotFoundException's when require attributes are missing from the xml.
  this seems a wrong to me. (i've left these for the moment since it's 
easier for people to examine the code when it's in cvs.) there are also a 
few Exception's thrown. i'd prefer specific subclasses to be thrown since 
this allows users (if they wish) to diagnose the original problem.

4. Delegate marker interface

this appears to be have been removed between the original and the update. 
it was still referenced in the tests. i've removed the reference from the 
tests for now. is this right?

5. Test cases

these wouldn't run against the updated code. the reason was a problem with 
local names verses qnames (on my machine). i've adapted the code so that 
they'll run on my machine but see next for discussion about namespaces.

6. Namespaces

it seems to me that there hasn't really been a lot of thought about 
namespaces. maybe more thought's needed on this subject before the plugin'
s code could be released. one option might be to allow the names of the 
plug-in element to be varied by the user. another might be to create a 
namespace for the plugin element to live in.

7. Logs

at the moment, each plugin class uses it's own and the log cannot be set. 
i'm not in favour of this pattern for several reasons. most rules 
implementations use the digester log and i'd prefer to switch as many 
classes as possible to use the log and give the others setters and getters.

- robert

On Thursday, October 2, 2003, at 03:46 AM, Simon Kitching wrote:

> On Wed, 2003-10-01 at 17:59, Simon Kitching wrote:
>> Hi,
>>
>> Many many moons ago, I proposed a "plugins" extension for digester.
>>
>> It is now ready for the world [yes, yes, brave words I know :-]
>
> Follow-up comments:
>
> *
> The package overview is accessable via this direct link:
> http://issues.apache.org/bugzilla/showattachment.cgi?attach_id=8410
>
> *
> I always wanted class PluginRules to be a decorator for other Rules
> classes rather than reimplement RulesBase functionality. It hasn't been
> possible in the past, because I needed to use a CursorableLinkedList (or
> equivalent) to hold the rules. However I realised this morning in the
> shower (true!) that given recent changes to the way PluginCreateRule is
> implemented, it is now possible to do so.
>
> I have attached an updated (and smaller) code tarfile to the bugzilla
> entry http://issues.apache.org/bugzilla/show_bug.cgi?id=23537.
>
> *
> Currently, PluginCreateRules creates a new Rules instance each time it
> fires, to hold the custom rules associated with whatever concrete class
> the user has specified. As it is expected that the same concrete class
> will be reused often (just see the examples), a cache could be created
> to hold these rules objects. I would rather hold off doing this
> optimisation, though, until I know whether plugins will be accepted into
> the digester tree or not.
>
> *
> Sorry about my coding style creeping into the posted code a little.
> In particular, I add underscores to the end of instance variables, like:
>   private String foo_;
>   public void setFoo(String foo) { foo_ = foo; }
> Habits are hard to break...
>
> *
> Calls to log.debug still need to be wrapped in if(debug) for
> performance, like in the rest of Digester code.
>
> Cheers,
>
> Simon
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message