Hi Clay, Just a couple follow-up comments below. Thanks for taking the time to address even the extraneous stuff.
- Keith Clay Baenziger wrote: > Hi Keith, > Thank you for the review! Comments in-line. New webrevs at: > http://cr.opensolaris.org/~clayb/python2.6/webrev2_diff/ > http://cr.opensolaris.org/~clayb/python2.6/webrev2/ > > Thank you, > Clay > > On Thu, 5 Nov 2009, Keith Mitchell wrote: > >> Hi Clay, >> >> My usual disclaimer applies - please don't hesitate to defer/ignore >> 'cosmetic' suggestions that are too big/risky for the scope of the >> 2.4->2.6 change. Given the indicated time constraints, I tried to >> really stick to modified code, though additional comments can be >> found in a separate section of my code review.... >> >> Thanks, >> Keith >> >> 78, 86: Use of str.ljust (and/or str.rjust) would be simpler and more >> appropriate (here and in several changed spots below) >> http://docs.python.org/library/stdtypes.html#str.ljust > > Nice, this is a more readable way to approach this. My question is > would this all still be better done with printf since printf style > format strings are so ubiquitous to so many? > > I don't really see line 86 being that well served by this, as I would > have to do the odd "-".center(length, "-") which, to me, seems goofy. > However, you can see there's a number of other places this worked well. You're right, format strings are more appropriate (and easier to read) in several of those cases. It's worth mentioning that printf style format strings seem to be going 'out-of-style' for Python 2.6 and 3.0. That's a conversation for another day, I think. > >> 399-409: Should be changed to use the 'if key in dict:' syntax > > Good catch I must have been playing obscure Python when I wrote that... > I used: > if not response.haskey(<long > key>): > [...] > > As this was more readable than: > if <long key> > not in response: > [...] > I agree with that assessment. Looking at that code, I'm curious if it does what we think it's doing... We 'yield str(colName)', yet, depending on the result of 'haskey', colName may or may not get 'MIN' and 'MAX' stripped out of it. It almost seems like 401-402 should be above 399, so that MIN/MAX are consistently stripped out before the yield statement, but perhaps the original behavior is intended? On a different note (and yes, I realize I'm now offering a completely different suggestion for this code block than I just did), it seems like 399-403 could be further collapsed with the use of dict.setdefault(), e.g., response.setdefault(colName.replace('MIN', '', 1).replace('MAX', '', 1), 1) This will set "response[colName] = 1" if colName is not in the dictionary, but leave it be otherwise. > >> list-manifests.py: >> >> 198: "for cri in criteria:" > > This needs to be criteria.keys() as this is an sqlite3 object which > does not behave as a dictionary. Ah, thank you. Subtle differences. >> webserver.py: >> >> 124: Could just be "for crit, value in critPairs.items():" > > It could be, however, this is an sqlite3 object which doesn't support > items(). I've added a comment. Thanks for clarifying. Seems like sqlite3 rows have a strange half-list/half-dictionary syntax which is confusing if you don't know what you're looking at. > >> 215, 289, 306, 316, 335: Can we use the @staticmethod decorator >> instead? Also, staticmethods should not have a 'self' parameter. > > Yes; this was my first Python and mostly first OO program. This was > completely superfluious. No worries! I'm sure I'll have just as many interesting twists in my code...
