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