Code review comment for lp:~rvb/launchpad/activereviews-bug-867941-eagerload3

Thanks for the review Brad!

> * Can branch_ids = map(itemgetter(0), result.get_all()) be rewritten as
> branch_ids = [res[0] for res in result.get_all()] ? If so, all
> things being equal, I think it is more readable.

True. Although you will admit that using map/itemgetter is cooler ;).

> * Actually now that I see createBranchChain it looks like
> createdStackedOnChain is redundant. Care to promote createBranchChain to the
> factory.py and re-use it?

Good idea.

> * As to your question about visibility of stacked on branches, if you can
> write a test to prove your assertion then I'd be happy for you to delete the
> extra step. But the test must be there to ensure we don't have a regression
> that will allow private branches to leak.

Good point, I'll keep the assertion for now.

> * Sorry I have no ideas on your 'column1' question. Perhaps you can ask stub
> later.

I suppose it'd good enough to be landed as is but I'll ask stub/jtv.