commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simone Tripodi <simone.trip...@gmail.com>
Subject Re: [digester] help on graduating at-digester sandbox to current implementation
Date Mon, 25 Jan 2010 07:15:09 GMT
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.
Thank once again, best regards,
Simo

http://people.apache.org/~simonetripodi/


On Mon, Jan 25, 2010 at 3:59 AM, sebb <sebbaz@gmail.com> wrote:

> On 24/01/2010, Simone Tripodi <simone.tripodi@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message