Hi Craig,
looks good!
Two minors:
- The patch file misses a newline at the end of the file which causes
problems when applying the patch. Could you please check whether file
companyNoRelationships.xml has a newline at the end?
- Do we use upper case letters for names of constants? If yes you might
want to change the name of factoryPropertyName and
defaultFactoryClassName in class CompanyFactoryRegistry.
Regards Michael
Hi Michael,
Thanks for the comments. A final code review is attached. I'll send
the xml file updates as a separate review.
------------------------------------------------------------------------
On Sep 5, 2005, at 9:39 AM, Michael Bouschen wrote:
Hi Craig,
I like the idea of using the xml bean factory instance pattern
instead of the static bean factory. I was not aware that I can add a
bean instance to the CompanyModelReader using an API such as
addSingleton instead of defining in xml. This solves the problem of
attaching the current pm to the company factory which is then used by
the CompanyModelReader when creating pc instances.
A few remarks about the patch:
- Today CompanyFactoryRegistry.registerFactory takes the name of the
company factory class as an argument. This means all the callers need
to get the value of the system property
jdo.tck.mapping.companyfactory and pass this to the call. Would it
make sense to add another method registerFactory taking just the PM:
public static final String companyFactoryClassName =
System.getProperty("jdo.tck.mapping.companyfactory");
public static void registerFactory(PersistenceManager pm) {
registerFactory(companyFactoryClassName, pm);
}
This keeps the handling of the property
jdo.tck.mapping.companyfactory local in class CompanyFactoryRegistry.
Good.
- Class CompanyModelReader should define a constant for the name of
the companyFactory bean in the xml:
public static final String COMPANY_FACTORY_BEAN = "companyFactory";
It is used in the addSingleton call in method configureFactory.
Right.
- The patch includes a changed version of derby.property where you
uncommented the special Mac property for derby. I guess you are not
going to check in this change, correct?
Correct.
Regards Michael
Javadogs,
Please check out the Wiki page
http://wiki.apache.org/jdo/PersistentInterfaces and this patch.
I've tested the companyNoRelationships.xml but haven't updated the
other testdata files, pending a review. The idea is to replace the
testdata xml files with the factory concept so they can be used by
the standard CompletenessTest as well as the interface test.
Craig
=
------------------------------------------------------------------------
Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:[EMAIL PROTECTED]
P.S. A good JDO? O, Gasp!
--
Michael Bouschen [EMAIL PROTECTED] Engineering GmbH
mailto:[EMAIL PROTECTED] http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:[EMAIL PROTECTED]
P.S. A good JDO? O, Gasp!
=
--
Michael Bouschen [EMAIL PROTECTED] Engineering GmbH
mailto:[EMAIL PROTECTED] http://www.tech.spree.de/
Tel.:++49/30/235 520-33 Buelowstr. 66
Fax.:++49/30/2175 2012 D-10783 Berlin
