Yoav,

Very nicely done. Thanks.

Two small comments though. First, Actions MUST have a logger per
instance, not a static one.

So, for example,

  if(getNamingContext() != null) {
    LOGGER.warn("Overwriting existing naming context.");
  }

should be written as

  if(getNamingContext() != null) {
    getLogger().warn("Overwriting existing naming context.");
  }

Second, you might consider taking advantage of the new parameterized
printing methods. So, instead of

  getLogger().warn("Missing " + JNDI_ATTR + " attribute, ignoring.");

you can write

  getLogger().warn("Missing {} attribute, ignoring.", JNDI_ATTR);

The latter form usually performs much better when the logging
statement is disabled. In this case, when statements of level WARN are
presumably enabled, it does not make big difference.

If you agree with the above, I'll let you do the changes.

More below.


At 10:08 PM 12/6/2004, Shapira, Yoav wrote:

Hi,
After an incredibly long delay, I got around to implementing an initial version of this. I just committed it, along with a modified JoranConfigurator to use it.

Better late than never.

I suppose the next step is to enhance the Joran unit tests to use this action. However, I wasn't sure what the best way is to modify the tests such that they use JNDI but don't break for everyone else ;)

Yoav, do you have an idea how we could embed tomcat into our test environment? It's such a sorely missing item at this stage....


Yoav Shapira http://www.yoavshapira.com

-- Ceki G�lc�

The complete log4j manual: http://qos.ch/log4j/



---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to