[ https://issues.apache.org/jira/browse/JDO-591?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12652469#action_12652469 ] 

Craig Russell commented on JDO-591:
-----------------------------------

Nice work. Regarding the patch:

1. 
+EXC_GetEnhancerNoValidEnhancerAvailable=\
+There were no valid JDOEnhancer implementations identified in the CLASSPATH.

Could add a little color: 
+EXC_GetEnhancerNoValidEnhancerAvailable=\
+There were no valid JDOEnhancer implementations identified in the CLASSPATH. \
+ The file META-INF/services/javax.jdo.JDOEnhancer should name the implementation class.

2. Copy/paste error in JDOEnhanceException:
+     * Constructs a new <code>JDOReadOnlyException</code> with the
+     * Constructs a new <code>JDOReadOnlyException</code> with the
+     * Constructs a new <code>JDOReadOnlyException</code> with the
\ No newline at end of file

3. Javadoc is incomplete, missing @return tags for many methods.

4. Suggest adding some javadoc to setOutputDirectory
+     * Mutator to set the location where enhanced classes are written.
+     * @param dirName Name of the directory

+     * Mutator to set the location where enhanced classes are written.
+     * If this method is not called, classes will be enhanced in place, 
+     * overwriting the existing classes. If overwriting classes in a jar file,
+     * the existing files in the jar file will be written unchanged except
+     * for the enhanced classes.
+     * @param dirName Name of the directory

5. For addClass methods, the format of the class names should be specified (e.g. binary names http://java.sun.com/javase/6/docs/api/java/lang/ClassLoader.html#name use $ as the delimiter for inner class/interface names).

6. For the method 
+    protected static JDOEnhancer getEnhancer(ClassLoader loader) {

Shouldn't this method be public? It's a part of the public "interface".

7. Also, the use of null to mean "the context class loader" is non-standard in the JDOHelper pattern for class loaders. It would be better to use a different method to mean "use the context class loader, and use the existing method to get the context class loader in a doPrivileged block, e.g. 
+    protected static JDOEnhancer getEnhancer() {
+        return getEnhancer(getContextClassLoader());
+     }

8. The method loadClass is protected. You should probably use the existing JDOHelper method     private static Class forName( for this purpose.

9. This comment seems left over from a copy/paste:
// remember exceptions from failed pmf invocations

10. This comment is misleading. If there is no implementation, there shouldn't be a file named META-INF/services/javax.jdo.JDOEnhancer.
+    	 * The contents of the file is a string that is the enhancer class name, 
+    	 * null or blank.

Suggest:
+    	 * The contents of the file is a string that is the fully qualified enhancer class name.



> Enhancer Invocation API
> -----------------------
>
>                 Key: JDO-591
>                 URL: https://issues.apache.org/jira/browse/JDO-591
>             Project: JDO
>          Issue Type: New Feature
>          Components: api2
>            Reporter: Andy Jefferson
>            Assignee: Andy Jefferson
>             Fix For: JDO 2 maintenance release 3
>
>         Attachments: jdoenhancer-4.patch
>
>
> Having a standard interface to invoke the enhancer makes a lot of sense so we can have interchangeability of enhancers (for implementations that support BinaryCompatibility). 
> A start point (for discussions) could be
> java -cp classpath  {enhancer-class} [options] [jdo-files] [class-files]
>     where options can be
>         -persistenceUnit persistence-unit-name : Name of a "persistence-unit" to enhance the classes for
>         -d target-dir-name : Write the enhanced classes to the specified directory
>         -checkonly : Just check the classes for enhancement status
>         -v : verbose output
> This then allows enhancement of the specified classes, or the classes defined by the specified JDO files, or the classes defined by the specified persistence-unit.
> What other control would people like to see ? 

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.