Code review comment for lp:~cody-somerville/launchpad/fix-bug-297709

Hi Cody,

Thanks for taking on this branch -- we really appreciate the contribution.

In the future note we do like a merge proposal to include a cover letter with some background information on the task. See https://code.edge.launchpad.net/~bac/launchpad/bug-483607/+merge/14925 for an example. There is a bzr plugin at https://edge.launchpad.net/lpreview-body which makes creating the cover letter very easy. Just a FYI, now on to the review.

* We try to use en_US spellings, though we don't follow it strictly, so please s/favour/favor. Also I note another typo that is not your but it would be great for you to fix s/independant/independent.

* At line 74 could you explicitly set the pocket for ff_pub_warty so the reader doesn't have to know the default?

* The test has a lot of trailing whitespace. Please delete those.

* In the code you have:

from lp.registry.model.sourcepackage import (SourcePackage,
    SourcePackageQuestionTargetMixin)

Please move that first package to the next line, which is part of our coding standards. All of the packages should be sorted alphabetically too, which they are.

* Please remove the unnecessary parens from:

from lp.soyuz.model.publishing import (SourcePackagePublishingHistory)
from lp.soyuz.model.sourcepackagerelease import (SourcePackageRelease)

* Thanks a lot for getting rid of the magic numbers!

Again, thanks for contributing this fix. The change looks great overall and will be ready to land after making the minor changes. I'm marking the proposal 'Approved' since the changes are so straightforward and the changes don't need to be re-reviewed. I'm not sure if you have permission to land this yourself but I'll be happy to do so if you cannot.