Hi Clay, All looks good now!
Thanks, Keith Clay Baenziger wrote: > Hi Keith, > Thank you for this review. Again comments in-line. > Thank you, > Clay > > On Fri, 6 Nov 2009, Keith Mitchell wrote: > >> 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. > > Yes, I've avoided them further since their future is muddy. > >>> >>>> 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. > > No, my change is wrong! Looking at the original again, I believe it > should be: > if colName.startswith('MAX') or > colName.startswith('MIN'): > # we have reported this criteria do not repeat it > if response.haskey(colName.replace('MIN', '', 1).\ > replace('MAX', '', 1)): > continue > colName = colName.replace('MIN', '', 1) > colName = colName.replace('MAX', '', 1) > response[colName] = 1 > yield str(colName) > >>> >>>> 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. > > Yeah, it took me a minute to figure out what was up > >>>> 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. > > Yeah, I had pydoc out to figure out why iteritems didn't exit. > >>> >>>> 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... >>
