Merge lp:~cody-somerville/launchpad/fix-bug-297709 into lp:launchpad
Proposed by
Cody A.W. Somerville
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Brad Crittenden | ||||
Approved revision: | not available | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~cody-somerville/launchpad/fix-bug-297709 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
196 lines (+70/-55) 2 files modified
lib/lp/registry/doc/distribution-sourcepackage.txt (+48/-34) lib/lp/registry/model/distributionsourcepackage.py (+22/-21) |
||||
To merge this branch: | bzr merge lp:~cody-somerville/launchpad/fix-bug-297709 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brad Crittenden (community) | code | Approve | |
Julian Edwards | Pending | ||
Review via email: mp+14929@code.launchpad.net |
Commit message
Fix latest_
To post a comment you must log in.
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.sourcepac kage import (SourcePackage, geQuestionTarge tMixin)
SourcePacka
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.publishin g import (SourcePackageP ublishingHistor y) model.sourcepac kagerelease import (SourcePackageR elease)
from lp.soyuz.
* 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.