Hi Valentin Thanks much for the valuable feedback. See replies in line below
Lin On Sat, May 8, 2010 at 11:54 AM, Valentin Mahrwald <[email protected]> wrote: > Hi Lin, > > the annotation code looks cool, will be well cool when blueprint xml is no > longer needed. Not sure if we 'll have the equivalent of annotation for all blueprint definition xmls, but hopefully for majority of them soon :) > > Some comments I have (in particular order): > > - how does field injection work in the current code, I couldn't see that you > set the extension attribute anywhere? Field injection is currently processed when processing the class that has @Bean annotation, the code checks if any of the declared fields have @Inject annotation. @Inject can be used on Field, Constructor and Method. > > - what is the intention behind giving the scanner service service.ranking=0, > I thought 0 was the default? No particular intention... since it is the only scanner service avail so it doesn't really mean anything right now :-) > - might it be better to allow the blueprint level settings > (defaultActivation, defaultTimeout, defaultAvailability) in the bundle > manifest as opposed to an annotation on a random class? Yes, that is certainly feasible, and may make more sense since the manifest is at the bundle level :) > - as far as I understand the FactoryMethod annotation, this annotation is > going to be used on the factory class, right? Does this mean that this only > works for factory classes using annotations (and in the same bundle), also > how does this work with multiple factory methods like > java.util.concurrent.Executors? Good question. I didn't think I work out issues with FactoryMethod annotation in implementation and that is why I commented out the annotation in AccountFactory.java. Seems to me it make more sense to allow users to specify the factoryMethod attribute and its arguments in @Bean annotation. I didn't go this route in the first place, as I thought it was pretty hard to specify such an annotation from a user's point of view. > - the Reference and ReferenceList annotations look a bit strange with an > annotation target of type because to me that suggests you can only use them > if your bundle contains the interfaces for any services you want to inject > via annotations. > > What about supporting annotation like > > class ServiceUser { > > @InjectReference( > ... > ) > private Service service; > } I like that, I think this is more reasonable. I'll see if we can use the existing @Inject annotation since it has an attribute ref, or create a new @InjectReference annotation. > - also the bind and unbind methods look awkward being marked as Strings in > the ReferenceListener annotation as opposed to having separate annotations > like Init and Destroy? (Same for registration listeners :) > > > Maybe (although that works only for annotation based reference listeners), > and similarly for RegistrationListeners: > > class ServiceUser { > > @InjectReference( > listener=Listener.class > ) > private Service service; > } > > @ReferenceListener > class Listener { > @Bind > public void bind() { ... } > > @Unbind > public void unbind() { ... } > } I think I didn't use @Bind or @Unbind annotation as I was not sure if there can be multiple bind methods in a class like the factory method. (The BindingListener.java seems to indicate that there can be multiple.) The init or destory I assume would be unique in the class. > Apologies for the amount of questions ;) No prob at all. :) Glad you reviewed it and appreciated the feedback! Lin
