Hi Keith,
Keith Mitchell wrote:
>
>
> Sundar Yamunachari wrote:
>> Jan,
>>
>> *usr/src/lib/install_utils/install_utils.py*:
>>
>> Suggestion:
>> 432-436: The checking of log to none can be made similar to other
>> checking of log in lines 477-483, 490-495, and 517-522
>> if log == None construct could be changed to if log != None so that
>> it will be consistent
>
> Per PEP-8, it should actually be either "log is None" or "log is not
> None"
I like it and I changed the code accordingly :-)
> Additionally, also use "if some_var" instead of "if len(some_var) > 0"
> - sequence types (lists, strings, etc.) evaluate True when their
> length is non-zero, and False if they're empty (this is also a PEP8
> guideline)
My apologies, but I am not going to take this suggestion -
PEP8 guideline (almost seems like our new Bible ;-) also states
at the very beginning:
...
Two good reasons to break a particular rule:
(1) When applying the rule would make the code less readable, even for
someone who is used to reading code that follows the rules.
(2) To be consistent with surrounding code that also breaks it (maybe for
historic reasons) -- although this is also an opportunity to clean up
someone else's mess (in true XP style).
...
Mainly because of (1), since I do think that following is less readable
if fl_cmd_finished and stderr_buf:
than the original
if fl_cmd_finished and len(stderr_buf) > 0:
>
> Additionally, I think the code from 470-495 and 514-522 or so could be
> greatly simplified by using the builtin splitlines function, which
> splits a string at newline characters and returns them in an array:
> for line in some_long_string.splitlines():
> log(line)
> However, I haven't taken a deep look at this code, so if I'm
> oversimplifying just ignore me.
The problem here is that we need to keep the last line in the buffer
if it is incomplete (not terminated with '\n') and not log it -
I think (I might be wrong since I haven't tried) that the suggested
change wouldn't address this aspect.
Thank you for looking at this !
Jan
