You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jdo-dev@db.apache.org by "Craig Russell (JIRA)" <ji...@apache.org> on 2008/12/02 20:28:44 UTC
[jira] Commented: (JDO-591) Enhancer Invocation API
[ 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.