ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Costin Manolache <cmanola...@yahoo.com>
Subject Re: antlib / proposal of Peter Reilly
Date Sat, 17 May 2003 18:59:08 GMT
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.


> 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.


> 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 ).




>> 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 ). 


Costin


Mime
View raw message