Hi Sebastien, Thanks. Please find my comments inline. Not much though :)
On Tue, Mar 18, 2008 at 3:46 AM, Jean-Sebastien Delfino < [EMAIL PROTECTED]> wrote: > Venkata Krishnan wrote: > >> 2) Unless I'm missing something, I don't think that you need to set the > >> targetNamespace of QualifiedIntent.qualifiableIntents, as it looks like > >> it's already read as a QName from the XML stream (and this QName does > >> not have to be in the current targetNamespace). > > > > > > First, I think that the Qualifiable intent needs to be in the current > > namespace since I I am not sure and the specs does not mention either, > on > > how we could represent a qualified intent whose qualifiable intent > belongs > > to a different namespace. So going with the assumption, in this context > the > > qualifiable intent needs to be assigned the targetnamspace. Only then > would > > it be correctly resolved during the resolution phase. > > > > Then I don't understand this code: > PolicyIntentProcessor:74 > qualifiableIntent.setName(getQNameValue(reader, > policyIntentName.substring(0, qualifierIndex))); > > which reads a QName from the XMLStreamReader. > > Either (a) the qualifiableIntent is always in the target namespace, and > then it's name is defined as an NCName and we shouldn't be trying to > read it as a QName, or (b) the qualifiable intent name is actually > defined as a QName and then it can belong to another namespace. > > If I understand it correctly, the policy-xsd defines these names as > QNames, leading me to believe that (b) is the correct option. Given the context of disussion in this thread (a) seems to be what it should be. When cleaning up I missed out this line and I will fix it rightaway :(. But it ends up working because the name is reset with the targetnamespace later. Why I say (a) is because we'd then have consistency with all intent names being NCNames. Ofcourse, this means that the qualifiable intents should also be from the same namespace. If we allowed intent names to be QNames then (b) would apply and we have the freedom of having qualifiable intents belonging to a different namespace than the targetnamespace. But that will get us back to the bunch of issues that has been discussed in http://www.mail-archive.com/tuscany-dev%40ws.apache.org/msg28653.html. I guess it can't be that qualified intents alone have names that are QNames and the rest will be NCNames. Thanks - Venkat > > > >> > >> 3) Finally a bigger change: it seems that you have > StAXArtifactProcessor > >> extensions for Intent, PolicySet etc... but these elements are not > >> extensions, so you didn't have to go through all that, as they are part > >> of the SCA core namespace. So, a much simpler approach would be to just > >> read them in, directly from SCADefinitionsProcessor. This is similar to > >> CompositeProcessor for example, if we had made a separate processor for > >> Component, we would have had to pass a lot of context to it. > >> > >> In short, it looks like you've created a maze of processor extensions > >> for things that didn't have to be extensions, and are now wondering how > >> to pass context through this maze :) > >> > >> The solution is simple, just don't make them extensions :) You can > >> either move this code to SCADefinitionsProcessor.read() or to private > >> methods of SCADefinitionsProcessor to which you'll be able to pass > >> whatever context you need. > >> > >> Hope this helps. > >> -- > >> Jean-Sebastien > >> > > > > This is the the first temptation that I went thro i.e. to merge all of > this > > processing into the SCADefinitionsProcessor similar to the > > CompositeProcessor. We did start with all of this in a single module > but > > somewhere down the line we decided to split them all into different > modules > > (can't quite remember and pull up that discussion around this). Ok, let > me > > get them back together for now. > > > > However, I am not sure how far we could go with this. You will agree > that > > the 'read' and 'resolve' methods of the CompositeProcessor are quite > bulky > > running to pages and contains quite a bit of state management. > > > > I'm not saying that you need to do all the processing in a single method > :), you can split it multiple methods if you like, but these methods can > be private methods, do not have to belong to the StAXArtifactProcessor > interface, and can take whatever context you want to pass to them. > > We just need to find the right balance between bulky methods and a maze > of small methods calling each other :) > -- > Jean-Sebastien > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > >
