Code review comment for lp:~wallyworld/launchpad/person-mergequeue-listview

Revision history for this message
Ian Booth (wallyworld) wrote :

Adding back comments from old incorrect merge proposal:

SteveK wrote:

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.

« Back to merge proposal