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

Revision history for this message
Brad Crittenden (bac) wrote :

Hi Raphaël,

* For createStackedOnChain(self, product) it might be nice to add another argument for the number of branches, defaulting to 5.

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

* Nice use of 'partial'. I learned something new!

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

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

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

review: Approve

« Back to merge proposal