Code review comment for lp:~mars/launchpad/turn-on-windmill-debug-logging

Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> mars, does reset_logging undo all of this logger buggery you're doing here?
<mars> rockstar, it does
<mars> rockstar, BaseLayer calls it as well, but I did this because it is more visible and explicit
<rockstar> mars, what does safe_hasattr do?
<mars> rockstar, doesn't eat exceptions. Builtin hasattr does (fun fun!)
<rockstar> mars, why not just use getattr?
<mars> rockstar, what line?
<mars> 96
<rockstar> mars, yes.
<mars> rockstar, it might be because hasattr will tell you that an attribute exists. getattr may return None, but you can't tell if that is the attribute's value, or the default argument.
<mars> or the default argument to getattr()
<rockstar> mars, okay. As long as you feel you're using the right call here, I'm happy with that.
<mars> rockstar, not my code, so it should work :)
<mars> rockstar, that is one of the code blocks I extracted into its own method
<mars> personal choice: I dislike 300 and 400 line method definitions, even if they are linear.
<mars> if there are comments that say "# now we do this" followed by "# then we do that", then it is pretty good sign that you can extract a method
<mars> but, like I said, that's personal preference
<rockstar> mars, I think you've made the code clearer by breaking it up.

review: Approve (code)

« Back to merge proposal