Jan Damborsky wrote:
> 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:
No need to apologize! Pointing at those PEP-8 rules is certainly a good
argument in this case.
>
> ...
>
> 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.
Ah, I see now. If you wanted to, you could store the splitlines as such:
lines = string_to_log.splitlines(True)
last_line = lines[-1]
for line in lines[:-1]:
log(line)
That would log everything but the last line, and save the last line for
later processing. You could then process the last line as before. The
True parameter to splitlines preserves the \n for each piece, e.g.:
"Hello\nThis is a string\n" becomes ["Hello\n", "This is a string\n"]
and
"Hello\nThis is a string too" becomes ["Hello\n", "This is a string too"]
Again, this is just a suggestion, feel free to take it or leave it.
>
> Thank you for looking at this !
You're welcome!
- Keith
> Jan
>
