db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michelle Caisse <Michelle.Cai...@Sun.COM>
Subject Re: Review of Matthew's patch
Date Fri, 30 Mar 2007 22:32:41 GMT
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 
> (matthew.adams@home).
>
> 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:Craig.Russell@sun.com
> P.S. A good JDO? O, Gasp!
>


Mime
View raw message