Code review comment for lp://staging/~fgiff/linaro-android-bot-review/update-gerrit-and-lava-integration-2

Revision history for this message
Frans Gifford (fgiff) wrote :

> I think "logger" is a bad name for what it does, it should be IntegrationInfoStore or something.

Sure. I called it logger as we were previously writing short logs to the filesystem. Since we now modify the records, it's not really a log any more, so a name change would help.

> testgerritconnection.py, being a unit test, should not expect any external context, or if it needs it, should pre-setup it.

I agree. If anything isn't set up, that's a bug.

> known_hosts and pub key are two different things, naming one another is confusing.

True, I'll rename it known_hosts.

> Well, I wondered why "{}".format(foo) syntax was used at all...

PEP 3101 'proposes a new system for built-in string formatting operations, intended as a replacement for the existing '%' string formatting operator.'

I don't have a particular preference for one syntax over another. This one sounded newer (less likely to get deprecated).

> Cannot continue with testing - doesn't work with Python 2.x (2.6.6 here), test outputs error message when there's actually no error.

I'll see what I can do about fixing that. It runs on my system, (python 2.7.1) so maybe there's a dependency on something in my environment.

« Back to merge proposal