|
Home > Archive > Apache JDO Project > April 2007 > Review of Matthew's patch
You are viewing an archived Text-only version of the thread.
To view this thread in it's original format and/or if you want to reply to
this thread please [click here]
| Author |
Review of Matthew's patch
|
|
| Craig L Russell 2007-03-31, 1:11 am |
| | |
| Matthew Adams 2007-04-02, 1:11 pm |
| Inline...=20
>-----Original Message-----
>From: Craig.Russell@Sun.COM [mailto:Craig.Russell@Sun.COM]=20
>Sent: Friday, March 30, 2007 2:09 PM
>To: JDO Expert Group; Apache JDO project
>Subject: Review of Matthew's patch
>
>Nice job Matthew. Good code, nice style, we should definitely keep =20
>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 =20
>a top level concept when finding PMF by name. This would be =20
>equivalent to the PUName that we've defined already. I'm not sure =20
>what we should do with the existing PUName. We could allow the user =20
>to specify the name as an attribute and check it where we check for =20
>PUName in attribute or property and allow only one of them.
>
Let's discuss.
>2. The same comment applies to the provider class. We might consider =20
>promoting the javax.jdo.PersistenceManagerFactoryClass to "provider". =20
>Similarly, let's look at all the elements of persistence-unit in =20
>persistence.xml.
>
Same response. Let's discuss.
>3. + catch (ParserConfigurationException e) {
>+ throw new JDOFatalUserException(
>+ msg.msg("EXC_ParserConfigException"),
>+ e);
>+ }
>
>Are you sure this is a user error? Could be a =20
>JDOFatalInternalException that should be reported to =20
>(matthew.adams@home).
>
Fixed in patch after my first commit.
>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 =20
>column number of the error. This should be available from the =20
>exception itself but for some reason is not in the message text.
>
Fixed in patch after my first commit.
>5. + catch (IOException ioe) { /* gulp */ }
>I don't remember talking about this. Shouldn't we wrap this in a =20
>JDOSomethingException?
>
This is only there when attempting to close the input stream in the
finally block. It's modeled after other code I saw that closed input
streams in finally blocks. I'll leave it unless you want to try to
throw exceptions in finally blocks that may be executed after an
exception is already being handled.
>6. Not that there's anything intrinsically wrong with your =20
>implementation, but I'd like to have a way for the jdo implementation =20
>to provide a different persistence.xml handler. Like add a method to =20
>JDOImplHelper to registerJDOConfigHandler, and refactor the existing =20
>handler to statically register itself. To do this, the jdoconfig =20
>handler needs to have a single instance with the methods you've =20
>created to do the parsing.
>
This would be a nice enhancement. I'll look into it. Did you mean
jdoconfig.xml instead of persistence.xml?
| |
| Matthew Adams 2007-04-02, 1:11 pm |
| Michelle, can you please check to see if this is now fixed?
>Some files have Windows line endings. This is probably okay=20
>as long as=20
>you have your CVS properties configured properly. See Craig's=20
>2/14/2007=20
>email to jdo-dev "Subversion eol-style"
>
| |
| Michelle Caisse 2007-04-02, 1:11 pm |
| Hi Matthew,
They spot check okay, but I don't have a good way of scanning them all.
My litmus test is to open the file in Notepad. If it displays all on
one line, you're okay. If it looks good, you're line endings are messed up.
-- Michelle
Matthew Adams wrote:[vbcol=seagreen]
> Michelle, can you please check to see if this is now fixed?
>
>
|
|
|
|
|