Code review comment for lp:~qzhang/lava-scheduler/retrieve-job-fail-reason

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

72 + JobFailReason.M("tar: You must specify one of the",

You can just say M as it is already in scope.

144 + if not 'server' in params or \
145 + not isinstance(params['server'], unicode):

The general consensus around using trailing \ is not to do it. Please use parentheses in this case.

>>> if (something \n rest of if):

+ JobFailReason.FAULT_LAVA, "deployment failed due to untar issue"),

You can say self.FAULT_LAVA, this is shorter and is usually preferred.

99 + errors = ""
100 + for r in self._health_check_reason:
101 + errors += "\n" + r.match_text + " -- " + r.reason
102 + return errors

Don't join strings like this in python. It is quadratic in complexity.
Instead use str.join(), it is linear in complexity and you get the number of glue elements right (you otherwise get the unneeded leading newline). Compare:

return "\n".join(
           ["{0.match_text} -- {0.reason}".format(r)
           for f in self._health_check_reason])

review: Needs Fixing

« Back to merge proposal