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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
