A few trivial comments:
Some files have Windows line endings. This is probably okay as long as
you have your CVS properties configured properly. See Craig's 2/14/2007
email to jdo-dev "Subversion eol-style"
In Bundle.properties - You have
EXC_DuplicateRequestedPUFoundInDifferentConfigs, but elsewhere
PersistenceUnit is spelled out. Should be consistent.
Constants.java- There's a typo in comments, replicated ~20 times by
cut & paste: * The name of the persitence manager factory ...
I agree with Craig that your code is very nice.
-- Michelle
Craig L Russell wrote:
Nice job Matthew. Good code, nice style, we should definitely keep you
on the team. Any more where you come from?
I think this is good to check in. We can resolve details with patches.
1. I'd like to have a name attribute for the PMF. Seems like this is a
top level concept when finding PMF by name. This would be equivalent
to the PUName that we've defined already. I'm not sure what we should
do with the existing PUName. We could allow the user to specify the
name as an attribute and check it where we check for PUName in
attribute or property and allow only one of them.
2. The same comment applies to the provider class. We might consider
promoting the javax.jdo.PersistenceManagerFactoryClass to "provider".
Similarly, let's look at all the elements of persistence-unit in
persistence.xml.
3. + catch (ParserConfigurationException e) {
+ throw new JDOFatalUserException(
+ msg.msg("EXC_ParserConfigException"),
+ e);
+ }
Are you sure this is a user error? Could be a
JDOFatalInternalException that should be reported to
([EMAIL PROTECTED]).
4. + catch (SAXException e) {
+ throw new JDOFatalUserException(
+ msg.msg("EXC_ParsingException", url.toExternalForm()),
+ e);
The SAX parser exception message should include the line number and
column number of the error. This should be available from the
exception itself but for some reason is not in the message text.
5. + catch (IOException ioe) { /* gulp */ }
I don't remember talking about this. Shouldn't we wrap this in a
JDOSomethingException?
6. Not that there's anything intrinsically wrong with your
implementation, but I'd like to have a way for the jdo implementation
to provide a different persistence.xml handler. Like add a method to
JDOImplHelper to registerJDOConfigHandler, and refactor the existing
handler to statically register itself. To do this, the jdoconfig
handler needs to have a single instance with the methods you've
created to do the parsing.
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!
