On Sun, 2005-05-22 at 18:38 -0700, Brian Stansberry wrote:
> It's a good point; the "assumed" category names in the
> various isXYZAvailable method is my biggest dislike
> about the patch I submitted. Actually, if an approach
> like the patch were adopted, I think we should
> consider marking those methods as deprecated (they're
> not used in the patched LogFactoryImpl itself) and
> dropping them at the next release with a point scope
> that allows it.
Ah - I see. With your approach, you just pass the name that the user
provided, so don't need a dummy category name at all. Fine.
Regarding deprecation, if we're going to change the logic flow within
the LogFactoryImpl such that the existing isXYZAvailable methods are no
longer called, then to me that's *just as much* an incompatible change
as removing the methods.
Removing the methods potentially causes subclasses to not compile/link,
but leaving them there and not calling them causes subclasses to
*logically* fail, which in my view is even worse because it's not
visible.
I'd be happy with including this approach in the next release, removing
the redundant isXYZAvailable methods and calling the next release 1.1.
In practice, I'm sure no-one *does* subclass LogFactoryImpl so there's
no problem. In fact I'd be happier with this than a 1.0.5 where the
methods exist but don't do anything.
>
> In general, if any significant work on LogFactoryImpl
> is done I think a review of its API would be good.
> This seems to me to be the kind of class that if
> someone wants a different implementation, they should
> write a different implementation and not count on a
> lot of behaviors exposed by a superclass.
I'm working on a major refactoring proposal now, that will not just
review but gut LogFactoryImpl :-).
>
> Looking into it more, I remembered that
> LogFactoryImpl.getInstance(Class clazz) just calls
> getInstance(clazz.getName()) and thus already creates
> a requirement that Log implementations support fully
> qualified class names. So any adapter that doesn't do
> this must be designed to work with a custom
> LogFactory. So, the potential list of users affected
> by this issue is smaller -- only those with custom
> LogFactory implementations and Log implementations
> that don't support class names.
Yes, good point. We should really document that in the Log interface. In
order to be able to swap in/out alternative logging implementations,
there does need to be a "standard" for category names that the concrete
Log classes can all handle. It's implicit in the whole concept of JCL
(well, any logging wrapper).
> Half-baked idea to throw out before I run. Add to
> LogFactoryImpl:
>
> /**
> * Returns a category that can be used to test whether
> * an instance of a <code>Log</code> can be created.
> * <p>
> * This default implementation returns the fully
> * qualified name of this class. Subclasses should
> * override this if they support <code>Log</code>
> * implementations that cannot handle this category.
> * </p>
> */
> protected String getTestCategory() {
> return getClass().getName();
> }
>
> All the various isXYZAvailable methods would call this
> method when creating a test instance.
No need I think. Your code in bugzilla which simply uses the first log
category passed in by the user seems fine to me.
One minor issue: if the user passes in an "invalid" category name of
some sort, then we might assume that the logging implementation is not
available. Not a likely problem though.
And as you point out above, JCL effectively imposes its own standard on
what is and is not a valid category name anyway, so if the above is
really considered a problem then we can just declare that "" is always a
valid category name that returns the "root" category, and all Log
subclasses need to handle that category name.
Hmm.. what about when a logging implementation is available, but
attempting to create a logger fails, eg because the underlying
implementation's config file is bad. If we test for the existence of the
logging lib by creating a logger, then we would fall back to other
logging libs in this case - is that what we want?
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
