Mailing-List: contact jdo-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: jdo-dev@db.apache.org
Received-SPF: pass (herse.apache.org: local policy)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
  s=s1024; d=yahoo.com;
  h=X-YMail-OSG:Received:X-Mailer:Date:From:Subject:To:Cc:MIME-Version:Content-Type:Message-ID;
  b=nAIsz9YQyI/foad5Ni3KMGKI+ofEQXDT/r4Lh/ykU81QlWUo1CJakjirScH9pEywk/523LjiioRQez9YD4JCuI8bUTbGVKB41Dr+sDO4MuUn/GlLZkgwVC/2FnijgtUnZ+ZT4O4iIh+xy9MyfZAM6Ca3em2tjwjunXTHiqVW5LU=;
Date: Fri, 30 Mar 2007 14:55:14 -0700 (PDT)
From: "Matthew T. Adams" <matthewadams@yahoo.com>
Subject: Re: Review of Matthew's patch
To: Michelle.Caisse@sun.com, Apache JDO project <jdo-dev@db.apache.org>
Cc: JDO Expert Group <jdo-experts-ext@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii
Message-ID: <21832.78865.qm@web30806.mail.mud.yahoo.com>

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 <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_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!
>