Code review comment for lp:~le-chi-thu/lava-dispatcher/improve-logging-2

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

Hi ChiThu

59 - rc = session.run('lava-test help', response="list-test", timeout=60)
60 + rc = session.run('lava-test -h', response="list-test", timeout=60)

I understand this is not related to logging

294 + def _set_logging_level(self):
295 + # set logging level is optional
296 + try:
297 + logging.root.setLevel(self.logging_level)
298 + except :
299 + pass

I understand the intent was to ignore missing entries in the job file (and in that case accessing self.logging_level would raise KeyError). I think this is somewhat confusing and I'd suggest wrapping that inside the def logging_level(self) property method. To users of this class this should be a normal property, not something that may raise an unexpected KeyError. In addition, I think we should validate the values of logging level that are passed. Instead of using raw integers that few users will understand (is 0 more verbose than 100?) I would just use the standard set of logging.XXX names {DEBUG, INFO, WARNING, ERROR, CRITICAL}.

I can show you how to make code like that elegant and how to validate the input quickly.

« Back to merge proposal