Code review comment for ~bladernr/plainbox-provider-checkbox:1628264-virt-timeout

Revision history for this message
Rod Smith (rodsmith) wrote :

The Good
--------

I've run the modified script on a couple of systems, one of which supports virtualization and the other of which does not, and I got expected results both times:

* https://certification.canonical.com/hardware/201701-25363/submission/120327/test/380/result/8905819/
* https://certification.canonical.com/hardware/201409-15506/submission/120330/test/380/result/8906798/

I have not tested every possible failure mode, though.

The Bad
-------

Both "pylint" and "flake8" produce large numbers of errors and warnings on this script. Most of these are for existing code, so it's unfair to ask you to fix them on this submission; however, there were a few complaints about code submitted here. From the pylint output:

C:443, 0: Exactly one space required after comma
    def log_check(self,stream):
                      ^ (bad-whitespace)
C:499, 0: Exactly one space required around assignment
                self.elapsed_time=0
                                 ^ (bad-whitespace)
W:529,30: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)
W:499,16: Attribute 'elapsed_time' defined outside __init__ (attribute-defined-outside-init)

If you prefer to file a bug report about the large number of complaints by these tools and leave these as-is, I'm willing to let this slide, since they seem to be trivial things that don't affect the program's ability to work, and they're a drop in the bucket amidst the many similar complaints by pylint and flake8.

The Question:
-------------

Finally, concerning the while/else clause on lines 501-524: It seems that a lot of code is duplicated from the main while loop in the else clause. The main difference seems to be that, in the else clause, the output of debug_file.read() is saved to the variable stream so that it can be logged if there's an error. Wouldn't it make more sense to simply store output in the stream variable in the main while loop and then log it only if there's an error? If there's some reason this doesn't work, then I'll pass it as-is; but if I'm not overlooking something, it appears that the code could be made cleaner by revising this logic.

review: Needs Information

« Back to merge proposal