I really like this, Thank you.
On May 8, 2010, at 9:17 PM, Lin Sun wrote: > 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 Johan Edstrom [email protected] They that can give up essential liberty to purchase a little temporary safety, deserve neither liberty nor safety. Benjamin Franklin, Historical Review of Pennsylvania, 1759
