Thanks, Oystein, for the quick turnaround. Your comments involve some
minor changes. I can go ahead and do this and submit an updated patch
ASAP. Someone let me know if you'd rather take what I have now -- I am
not clear on the constraints of the release schedule.
Some comments interspersed below...
David
�ystein Gr�vlen wrote:
"DVC(" == David Van Couvering (JIRA) <[email protected]> writes:
DVC(> [ http://issues.apache.org/jira/browse/DERBY-243?page=all ]
DVC(> David Van Couvering updated DERBY-243:
DVC(> --------------------------------------
DVC(> Attachment: DERBY-243.diff
DVC(> Updated patch for this issue. Here is a summary of what I changed. A lot of what I did was remove code I had put in,
DVC(> so I think you'll see a much simpler patch.
Looks very good. I am running derbylang on it know and will let you
know about the results when it is finished.
A few minor comments:
- EmbedPooledConnection.toString(): You do not have to create an
Integer object to convert int to String. You can use the static
Integer.toString(int) function.
Duh! :)
- There is some unecessary white space changes. Adding or
removing empty lines are OK, but the use of "svn blame" (or
praise) becomes less useful if lines that are unrelated to the
check-in are changed. Even worse, checkDataSource30.java only
contains white space changes. This gives the false impression
that this test is changed by your fix.
OK, I didn't think this was too important, but I'll see if I can fix this.
With checkDataSource30, that should just not be there. I will revert
back to the original one.
- The Javadoc for checkDataSource.checkToString(Connection) seems
to lack the word "sure".
OK
- You have changed the checkDataSource.main test so it may throw
an Exception. What is the purpose of that. Will the test
harness handle the Exception?
It used to throw an exception. I just changed it to print the stack
trace prior to throwing the exception to the caller because I wasn't
getting a stack trace when I was hitting bugs in the tests.
David
