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.
