On Tue, 12 Mar 2002, Conor MacNeill wrote:
> I've done cleanup on the ProjectHelper changes and I have some
> comments/questions. I'm not sure how much a pluggable ProjectHelper
> implementation is required.
>
> 1. I don't know why ProjectHelperImpl extends ProjectHelper. I had expected
> some
> sort of separate abstract class or interface that the ProjectHelperImpl would
> extend and which other ProjectHelperImpls would also extend. Right now
> ProjectHelper is acting as both the factory and base class for
> ProjectHelpers. I
> think it would be better to separate this out. Since the previuous
> implementation of ProjectHelper kept most methods private this should not be
> difficult.
We could have a ProjectHelperFactory, to create the ProjectHelper
instance. But do we need it ? My feeling is the current ProjectHelper is
simple and clear enough - it has few static methods ( for convenient
access to various introspection features ), the (abstract) parse() method
that will be implemented in subclasses, and the code to find and load
the real implementation.
Given that this abstraction will be used in few very specialized cases
( to completely replace the xml processor ), I don't think we need to
make too much complexity out of it.
> 2. Why has parse become public? There is no need for this to be public. In
> fact
> the old ProjectHelper only exposed configureProject (beside the utility
> methods)
IMHO the 'right' way to use ProjectHelper is:
ProjectHelper impl=ProjectHelper.getProjectHelper();
// set/query impl options ( none for now )
impl.parse();
Static methods are nice sometimes, but both mechanisms are valid and many
people feel more comfortable using instance methods. My preference would
be to deprecate configureProject ( i.e. recommend people to avoid it ),
but I don't feel very strongly about it.
> 3. Why is getProjectHelper public? There is no need for users of
> Projecthelper
> to get the actual ProjectHelper instance being used. All they should be using
> is
> configureProject()
See previous.
If needed, we might have to add other methods, like isSax2Processor() or
isXmlBased().
Or createProject() - to allow the helper to use a specialized Project
impl.
> 4. Why is ProjectHelper's constructor public?
I changed it to protected.
> 5. If the user has specifically specified a Projecthelper instance via a
> property I don't think it should be ignored. The following should probably be
> fatal.
> } catch (SecurityException e) {
> // It's ok, we'll try next option
> ;
> }
Ok, that covered the case when ant is run in a sandbox and it doesn't have
permissions to read the system properties - probably doesn't make too much
sense in the case of ant, but it was quite important for Jaxp ( from which
I cut the discovery code ).
The idea was to fall back to the next discovery mechansim if the previous
fails - but in ant's case you are right, failing is better ( since the
various implmentations of ProjectHelper are not equivalent ).
I fixed that, thanks.
Costin
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
