Code review comment for lp:~al-maisan/launchpad/disabled-copyarch-519665

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

I have a few additional notes that Gavin thought were worth adding:

 * What did the pre-imp conclude about the problem of re-enabling archives? Is it not going to be a problem to un-suspend jobs when that happens?

 * In build_not_pending_and_suspended, the "not" is ambiguous in scope. Can't you replace this with two separate checks?

 * In production code we check "len(foo) == 0" to check for empty lists. But in tests it's nicer to compare "foo == []" so that a failure will give more information.

 * With assertEqual, pass expected value first and actual value second! Again, better failure messages.

Jeroen

review: Needs Fixing (code)

« Back to merge proposal