limpbizkit commented on revision r1186 in project google-guice.
Details are at http://code.google.com/p/google-guice/source/detail?r=1186
Score: Positive
Line-by-line comments:
File:
/trunk/extensions/persist/src/com/google/inject/persist/Transactional.java
(r1186)
===============================================================================
Line 26: * <p> Any method or class marked with this annotation will be
considered for transactionality.
-------------------------------------------------------------------------------
"Considered for transactionality" ? What the heck does that mean? It's so
weak
Line 27: * Consult the documentation on
http://code.google.com/p/google-guice for detailed semantics.
-------------------------------------------------------------------------------
<a href>
Link to the proper docs?
Line 30: * {...@code forAll(Matchers.annotatedWith(Transactional.class),
Matchers.any()} clause in your
-------------------------------------------------------------------------------
Why is it the user's job to do this?
Mismatched parens
Line 33: * Class level {...@code @Transactional} allows you to specify
transaction semantics for all
-------------------------------------------------------------------------------
<p>
Line 34: * non-private methods in the class once at the top. You can
optionally override it on a
-------------------------------------------------------------------------------
non-private or (non-private, non-final) ?
You could link to the AOP page on the Guice docs and list all of the AOP
limitations
Line 36: * but with methods marked {...@code @Transactional} will *not* be
intercepted for transaction
-------------------------------------------------------------------------------
*not* --> <strong>not</strong>
Line 53: * A list of exceptions to <b>not<b> rollback on. A caveat to
the rollbackOn clause.
-------------------------------------------------------------------------------
<b>not<b> --> <b>not</b>
Line 56: * The complement of rollbackOn and the universal set plus any
exceptions in the
-------------------------------------------------------------------------------
<p>
Line 61: Class<? extends Exception>[] ignore() default { };
-------------------------------------------------------------------------------
do ignored exceptions get rethrown?
File:
/trunk/extensions/persist/src/com/google/inject/persist/WorkManager.java
(r1186)
===============================================================================
Line 38: public interface WorkManager {
-------------------------------------------------------------------------------
This interface doesn't really feel like a "Manager" to me . . . rename to
UnitOfWork ?
Line 41: * Starts a Unit Of Work. Underneath, causes a session to the
data layer to be opened. If there
-------------------------------------------------------------------------------
should this be Unit Of Work or unit of work ?
Line 45: * Transaction semantics are not affected.
-------------------------------------------------------------------------------
<p>
Line 53: * <p>
-------------------------------------------------------------------------------
nit: <p> goes on the next line, immediately before the word "Transaction"
File:
/trunk/extensions/persist/src/com/google/inject/persist/finder/DynamicFinder.java
(r1186)
===============================================================================
Line 1: // Copyright 2010 Google Inc. All Rights Reserved.
-------------------------------------------------------------------------------
open source copyright headers?
Line 5: /**
-------------------------------------------------------------------------------
Javadoc is so passe
Line 8: public final class DynamicFinder {
-------------------------------------------------------------------------------
is this supposed to be an @interface ?
Respond to these comments at
http://code.google.com/p/google-guice/source/detail?r=1186
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en.
