On 25/01/2010, Simone Tripodi <simone.trip...@gmail.com> wrote: > Hi Sebb, > thanks a *lot* for your review, I'll start working on fixes asap!!! > And no, I didn't have the need to modify the digester original code, I just > added the 'annotations' package.
Good - that will make it much easier when it needs to be moved. > Thank once again, best regards, OK, no problem. > Simo > > > http://people.apache.org/~simonetripodi/ > > > > On Mon, Jan 25, 2010 at 3:59 AM, sebb <seb...@gmail.com> wrote: > > > On 24/01/2010, Simone Tripodi <simone.trip...@gmail.com> wrote: > > > Hi all guys, > > > I'm writing you because I really need your help on graduating my effort > > > spent on sandbox about the digester: I finally terminated porting my old > > > stuff[1] on the sandbox and implemented some test cases that show how to > > use > > > annotations on pojos. > > > I need your help on: > > > - reviewing the design; > > > - make the code more compliant to commons-digester; > > > - increasing test cases > > > - suggest me what I have to do to propose the sandbox merged in /trunk > > > - help me on writing the documentation, I'm not a native English speaker > > :( > > > :( > > > > > > Is anyone available to help me? I'll start writing an architectural > > document > > > to provide you better informations, and apologize in advance for my poor > > way > > > to explain concepts in English :P > > > > > > In the meanwhile, if you have spare time to have a look at the code you > > can > > > check it out from svn sandbox[2] > > > > I cannot comment on the design or behaviour, as I'm not familiar with > > Digester. > > > > However, I can comment on the coding style. > > > > There are a few missing @Override markers. > > > > @SuppressWarnings("unchecked") should be used as close as possible to > > the statement that causes the warning, and there should be a comment > > as to why the warning can be ignored. > > > > For example, in DefaultLoaderHandler.handle(), the annotation should > > be used on the first statement of the try{} block. Similarly for > > .MethodHandler.doHandle(), etc. > > > > There are a few private methods where the Javadoc is incomplete, e.g. > > MethodHandler.doHandle(). Either drop the Javadoc, or have something > > useful there. > > > > The Javadoc for MethodArgument() references a non-existent parameter; > > there are a few other Javadoc problems, e.g. the @see references. > > > > It would be useful to document which classes are intended to be > > thread-safe, and which are definitely not intended to be thread-safe . > > > > Could the AnnotationRuleProvider.init() method be dropped in favour of > > providing the parameters in the constructors? That would allow the > > fields to be made final, which would improve thread-safety. > > > > In the MethodArgument class, two of the methods return a pointer to > > the final Annotations array. This breaks containment, as it allows > > external classes to modify the array. It would be safer to return a > > copy of the array. This would also make the class immutable and > > therefore thread-safe. > > > > Findbugs warns that the field FromAnnotationsRuleSet.namespaceURI is > > never written. > > This must be a mistake. > > > > Having said all that, I think the general style seems better than the > > existing Digester code, which has a lot of problems, e.g. public > > static fields that are not final! [Any such issues should be fixed in > > trunk, not here]. > > > > By the way - did you need to make any changes to the existing Digester > > code? > > > > > Thanks in advance, all the best!!! > > > Simo > > > > > > [1] http://code.google.com/p/digester-annotations/ - still on google > > code, > > > do I have to drop it? > > > [2] https://svn.apache.org/repos/asf/commons/sandbox/at-digester/trunk > > > > > > http://people.apache.org/~simonetripodi/ > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
