Gosh, thanks, guys, for the compliments [sheepish grin with shoulder rai
se].

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 <Michelle.Caisse@sun.com>
To: Apache JDO project <jdo-dev@db.apache.org>
Cc: JDO Expert Group <jdo-experts-ext@sun.com>
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_DuplicateRequestedPUFoundInDifferent
Configs, 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!
>

[ Post a follow-up to this message ]