On 11/30/06, David Blevins <[EMAIL PROTECTED]> wrote:
Reviewed and applied. I have to say I liked the path quite a bit.
Simple, clean and with javadoc worthy of the website.
Just to add to what Dave has said. I was about to have committed it,
but was and am still actually having troubles with running OpenEJB
tests. Anyway, the comments are given here.
1. Create a patch that follows the pattern like - OPENEJB-<id>.patch,
i.e. OPENEJB-387.patch.
2. There's no need for doLoad = true; twice. Just check whether you
should skip a given url and go ahead otherwise.
3. I'd go for ConfigurationFactory.logger.info("Found " + type + " in
classpath: " + path); before if's
4. Unnecessary // path = null;
5. And why is this - IOException e1? Why is it e1? I know it's not
part of your patch, but then it's just a comment and I've spot it
while taking a look at your patch.
6. And the last but not least - why is this change?
} catch (IOException e1) {
e1.printStackTrace();
ConfigurationFactory.logger.warning("Unable to search
classpath for " + type + ": Received Exception: " +
e1.getClass().getName() + " " + e1.getMessage(), e1);
}
+
}
? What is the additional empty line for?
Just a couple of remarks before we let the patch rest ;-)
Jacek
--
Jacek Laskowski
http://www.jaceklaskowski.pl
