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
