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.
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. IStoreSelector) , which is deprecated-ish
* 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(
* 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.