Code review comment for lp:~thumper/launchpad/bugjam-619555-request-builds-logging

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

> Nice, straight-forward branch.
>
> The only thing I can suggest is that the conditional in TestLogger.log() is
> unnecessary. If args is an empty tuple, then "msg %= args" will leave msg as
> it was.

The MockLogger has a comment saying that they check whether args is empty, so that msg will not blow up if it contains formatting strings. It looks like logging.LogRecord.getMessage() has the same type of logic, so it would be worthwhile to keep.

I think that TestLogger should either subclass from FakeLogger, or TestLogger should be removed, and BufferLogger should be used in its place. We're beginning to get as many logger classes as Linux has text editors.

review: Approve (code)

« Back to merge proposal