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.
Hi Steve
I've done the requested fixes. However, I also looked again at the diff equeue. This is why the diff is so large. The other code
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_branchmerg
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: IStoreSelector) , which is deprecated-ish
> 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(
> * 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.