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

Hi Steve

Thanks for the review.

>
> Firstly, thanks for this awesome work! I've been looking forward to Merge Queues for a while.
>

Actually, rockstar did the modelling work and is the champion of this
stuff. My stuff merely piggy backs onto his :-)

> 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.

Yeah. It wasn't meant to be so big - the core code changes were quite
manageable. But by the time I finished writing all the unit tests, the
line count sky rocketed. Sorry.

> * import with_statement isn't needed any more, we're on Python 2.6!

Aaargh. That will teach me to cut'n'paste.

> * I'd suggest you run the import formatter over your branch.

I did that - utilities/format-imports is the right one to use? Not sure
what happened if there are still issues.

> * Why use Star Wars names in the tests, I'd prefer descriptive names, such as self.branch_owner, which is less distracting.

Ok. Others use names like eric which are equally non-descriptive :-) I
was trying to make writing tests a bit lest tedious :-)

> * You should use XXX, rather than TODO, and file a bug per the XXX Policy.

In this case, it's not so much a bug per say as something rockstar is
picking up and running with for the next iteration of work. So do these
collaborative development efforts still need a bug report for what is in
effect a handover of work?

> * getUtility can be imported from zope.component directly, and indeed you mostly are, except in one place.

Agggh. IDE auto import bites me again.

> * Investigate usage of IMasterStore, rather than getUtility(IStoreSelector), which is deprecated-ish

Will do. Thanks for the tip. I was basing my work on what I saw had been
done before.

> * 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.

Hmmm. rockstar did this in his branch and mine is based on his. But
because he had trouble getting his through ec2, I merged from his
working copy to bootstrap my work. I didn't actually add those packages.
So perhaps there's a merge issue biting us?