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