I also meant to say: - Please make sure that you include doc strings where appropriate.
--Mark Mark J. Nelson wrote: > John Sonnenschein wrote: >> Hello >> >> could someone do a review of http://cr.opensolaris.org/~error404/140/ >> for me >> >> That change passes all the tests /ws/scmtest/src/legacy/scm/tooltest >> can throw at it > > usr/src/tools/onbld/Checks/Comments.py: > > - I might be missing something here, but it seems like you're reversing > order of fields in the tuples that make up the list of cases: > > 137 com, id = case.split(' ') > 138 cases.append( (com, id) ) > > ... > > 143 for case in cases: > 144 id, desc = case > > - Why are you even splitting case in the loop on 143? You only use id > and desc to append to the nonexistent and nomatch error lists, whereas > above the code simply uses the case tuple (line 131-132, similarly for > the old code). > > - It seems like line 146 should be followed by a "continue." > > - For duplicate entries, you'll never get a match on line 156, right? > Using a single line as a basis for comparison, the old code checked each > duplicate instance, but the new code checks the aggregate of such > instances, and that seems wrong. > > - Why test the entire comment first, and then trim to 40 chars if that > fails? Why not simply test the first 40 chars? > > - In general, I'm not sure why you changed the structure of the "for > entered in insts:" loop. I know you need to change some of the details > to match the DbLookups.py changes, but the changes as they stand seem > less correct than the old code. > > usr/src/tools/onbld/Checks/DbLookups.py: > > 245 if int(fields[0]): > 246 continue > > ...really? Won't int() throw an exception if it gets a string that > doesn't represent an integer? What was wrong for testing for the string > "0"? > > 247 arc, case = arclist[data.index(line)] > > ...do you need to do this? Can't you just split fields[1]? > > --Mark >
