launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Hi Steve

I've done the requested fixes. However, I also looked again at the diff
as shown by the merge proposal and it seems there's a bit of a mix up
there. When I created the mp I included rockstar's branch as a
prerequisite to mine, but the diff contains some of his stuff eg
test_branchmergequeue. This is why the diff is so large. The other code
is also the culprit for the "import with_statement" stuff. So I would
love to find out what happened there.

On 02/11/10 13:22, Steve Kowalik wrote:
> Review: Needs Fixing code*
> Hi Ian,
> 
> Firstly, thanks for this awesome work! I've been looking forward to Merge Queues for a while.
> 
> I have some comments below:
> 
> * 1,500 lines? Why!? I'd suggest in future you look at splitting up work into two separate branches for plumbing and browser code, for example.
> * import with_statement isn't needed any more, we're on Python 2.6!
> * I'd suggest you run the import formatter over your branch.
> * Why use Star Wars names in the tests, I'd prefer descriptive names, such as self.branch_owner, which is less distracting.
> * You should use XXX, rather than TODO, and file a bug per the XXX Policy.
> * getUtility can be imported from zope.component directly, and indeed you mostly are, except in one place.
> * Investigate usage of IMasterStore, rather than getUtility(IStoreSelector), which is deprecated-ish
> * You're adding BeautifulSoup and soupmatchers to setup.py and versions.cfg, I'd suggest that get split out into an earlier branch so you can be certain they are available before landing this one.
-- 
https://code.launchpad.net/~wallyworld/launchpad/person-mergequeue-listview/+merge/39745
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/person-mergequeue-listview into lp:launchpad.