ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From peter reilly <peter.rei...@corvil.com>
Subject Re: antlib / proposal of Peter Reilly
Date Mon, 19 May 2003 08:46:39 GMT
Hi Costin,
I will reply to these e-mails separately, if this is ok.


On Saturday 17 May 2003 19:59, Costin Manolache wrote:
> Sorry for the late reply, I had almost no acces to internet ( or time )
> last week.
>
> My main concern is the same as Conor's - having this decoupled and done
> in few steps.
>
> > peter reilly wrote:
> >> On Thursday 15 May 2003 07:56, Conor MacNeill wrote:
> >>
> >> I would prefer to use the XML schema attribute for this.
> >
> > Mmmm, this would be confusing as the XML schema attribute
> > has an associated URI "http://www.w3.org/2001/XMLSchema" which
> > implies a lot more that a reference to an ant type.
>
> Schema-style attributes - maybe. XMLShema itself is one of the worst
> things in XML (IMO), I would preffer we stay as far away as possible - but
> I'm ok with using some ideas.
>
> >> I have done a quick review of you proposal. I wonder if we can split
> >> this into smaller chunks to make it easier to understand and review. For
> >> example, the onError stuff could be split out, as could the URL handling
> >> code for separate consideration. Smaller chunks are easier to handle.
> >
> > This is true, but difficult to do. Some of the implementations of the
> > different features change/improve if other features are present. For
> > example the implementation of "onerror" uses the new anttypedefintion
> > class. The implementation of the psuedo task "antlib" uses the add(Type)
> > mechanism rather than explicity stating addTypedef and addTaskdef, this
> > allowed other tasks that extend Definer to used in the antlib task (for
> > example a definer that wraps Runnable objects as ant tasks, or
> > a future implemation of roles).
>
> That means you have to start with add(Type), then anttypdef, then onerror.
>

I could do this, but it would take a little time... The implementation
of add(Type) has been different after each of my updates...

One reason I combined them all together was to get a feeling on the
solution when all the pieces were in place.

> > Also from a practical point of view, I find it difficult to maintain
> > multiple patched ant versions.
>
> I think we are talking about the main branch - so there is no
> "patched ant version" to maintain. From a practical point of view, it
> is much easier to review and accept smaller chunks.
>
> >> Anyway, it seems to me that you have combined the namespace URI and
> >> element names in certain places. Examples: In component helper changes,
> >> for method createComponent, you say
> >>
> >> the name of the component, if
> >> the component is in a namespace, the
> >> name is prefixed withe the namespace uri
> >> dropping the "antlib:" and adding ":".
> >>
> >> Why not pass the URI and local name to the component helper and not have
> >> to parse it out in componentHelper. Your approach also precludes URIs
> >> that contain ':' (I know you disallow these elsewhere but I don't see
> >> any reason to combine these, anyway)
> >
> > Initially I was going to do this, but here is a lot of places in the code
> > that assume that a task/type is mapped from a string and not from the
> > tuple
> > {ns uri, local name}.
> > J.Pietschmann suggested in
> > http://marc.theaimsgroup.com/?l=ant-dev&m=105233644225128&w=2
> > that one can use a mapping of {uri, name} to a string ns-uri+"^"+name
> > when migrating a project to namespaced XML.
>
> I agree using a combined NS + Element may be simpler.
>
> I would suggest ns-uri:name ( i.e. : instead of ^ ). It is easy to extract
> the name with lastIndexOf(). It's just cosmetic, of course.

This is exactly what my patch on friday does :-).

>
> > My current mapping is not good as it drops the antlib: prefix and thus
> > excludes other uri prefixes, so I will change this. The current code does
> > exclude URI that contain ":", this is a combination of a bug and by
> > design. The bug is that I should have used lastindexof and the design is
> > that the code only deals with NS URIs that start with "antlib:" for
> > type/task definitions. The code for the mapping should be done in one
> > place (maybe as a static method in ProjectHelper).
>
> In any case, "antlib:" or any prefix should _never_ be used in resolutions.
> Only the URI. The prefix is just a syntactic shortcut, the URI is the one
> that matters.
>
> >> I'm not sure where TaskAdapter went. createNewTask seems to return null
> >> if the class is not a Task - probably handled somewhere else.
> >
> > This is by design and for BC reasons. If the type class is not a Task,
> > and the type does not have an Adapter to a Task class, the
> > CM#createNewTask() will not create the class. <taskdef/> will does this
> > (for example the condition task), and the unit tests contain an adapter
> > for Runnable objects. The code in CM does not treat TaskAdapter as a
> > special case.
>
> I think <taskdef> should be treated as a special <typedef> with TaskAdapter
> as adapter.
>
> ( i.e. use <typedef> as the main definition mechanism, and taskdef as
> a shortcut for types with optional TaskAdapter adapters ).

This is exacty what "taskdef" is in the patch :-).  It derives
from typedef and sets the adapterClass to TaskAdapter, and the adaptoClass
to Task. The html page for taskdef in the friday patch makes this explicit.

I have some WorkInProgress code to allow this to be specified in a buildfile.

<extendtype name="taskdef">
    <typedef adapter="org.apache.tools.ant.TaskAdapter"
                  adaptto="org.apache.tools.ant.Task"/>
</extendtype>

"extendtype" is a WIP name for a cut-down templating feature, it
defines new types from other types with some attributes pre set.

>
> >> For the most part it looks OK to me. I'd need to look more closely to
> >> fully comprehend it but thought you might like some feedback.
>
> Same here. If it can be split into smaller pieces - you have my +1 on the
> overal proposal ( each piece will be reviewed separately and may need some
> changes based on the review ).
>

Cool, Peter.


Mime
View raw message