db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Van Couvering <David.Vancouver...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-243) connection toString should uniquely identify the connection
Date Wed, 08 Jun 2005 16:33:41 GMT
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...


Øystein Grøvlen wrote:

>>>>>>"DVC(" == David Van Couvering (JIRA) <derby-dev@db.apache.org>
>     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".


>     - 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.


View raw message