See comments inline. >>>>>>>>>>>> Army wrote (2005-05-11 16:18:42): > Bernt M. Johnsen wrote: > > >Hi all, > > > >Could somebody review this patch (my first)? Now all identifiers > >should have a max length of 128 (note: MAX_USERID_LENGTH is > >unchanged). > > I reviewed this patch and it looks good to me. I noticed that in the JIRA > entry for this bug, you had written that you wanted to do the following: > > [ begin quote ] > > 2) Ensure that all DB2 related constants are prefixed by DB2_ > There are now 6 constants which do not have the prefix: > MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, > MAX_USERID_LENGTH, MAX_DECIMAL_PRECISION_SCALE, DEFAULT_DECIMAL_PRECISION, > DEFAULT_DECIMAL_SCALE > These should get the DB2_ prefix if they are DB2-related (are they?) > > [ end quote ] > > Did you decide to not do this part?
I never got an answer, so I decided not to touch these constants. > For the record, the comments in the > code suggest that the first three of these > (MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, and > MAX_USERID_LENGTH) are definitely DB2-related, so adding the "DB2_" prefix > to those would make sense. Should I add "DB2_"-prefix to these constants, then? (and leave the latter-three unchanged?). > As for the latter three, one might assume that > they are DB2-specific since they were declared in the "DB2Limits.java" > file, but maybe that's not a valid assumption...anyone out there know?? On > the one hand, I think these "DB2_" prefix changes could easily be checked > in later with a different patch, if someone wants to do it. On the other, > it might be less inuitive to people browsing changes to see a solo patch > that adds a "DB2_" prefix to some constants in "Limits.java"--at the very > least, the patch to do so would have to be very well-commented, or else it > could lead to confusion. > > >NOTE: I couldn't get svn diff to reflect that I have renamed one file > >(done with svn rename). If it is a problem, I can undo the rename and > >submit a new patch. > > I did have problems applying the patch to the my local codeline, and I'm > pretty sure it was because of this. In order to get the patch to apply, I > manually copied "DB2Limit.java" to "Limits.java"--after doing that (and > only that), I was then able to successfully apply the patch. So whoever > commits this might need to do the same, or else use some other trick to get > the patch to apply correctly. Seems to me that this is a weakness in the way we submit pathces (probably a weakness in svn). > After applying the patch I ran a couple of the tests that were affected, > and they passed as expected. > > One last question: did you have a chance to run the "derbyall" suite on > this to make sure you caught all of the relevant tests? I'm guessing that > you did based on the number of masters that you updated, but it'd be nice > to confirm--typically when you submit a patch, it's good to indicate what > tests you ran (and on what platforms/JVMs) just so that reviewers know you > actually tested your patch before posting it (which you clearly > did!) ;) Yes, I ran derbyall: Java Version: 1.4.2_02 Java Vendor: Sun Microsystems Inc. OS name: Linux OS architecture: i386 OS version: 2.4.20-31.9 > > All in all, looks good to me. Thanks. -- Bernt Marius Johnsen, Database Technology Group, Sun Microsystems, Norway
