Gosh, thanks, guys, for the compliments [sheepish grin with shoulder raise].
I'll incorporate your comments. I've fixed a few typos I saw already since this morning's conf call -- I'll correct them and issue another patch when I think it's ready. -matthew ----- Original Message ---- From: Michelle Caisse <[EMAIL PROTECTED]> To: Apache JDO project <[email protected]> Cc: JDO Expert Group <[EMAIL PROTECTED]> Sent: Friday, March 30, 2007 3:32:41 PM Subject: Re: Review of Matthew's patch 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! >
