launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

How embarrassing.  I typed this into an unrelated bug instead of into this MP:

I'm very glad you're fixing this, and my first thought about your approach was "oh great!"

But I'm having two second thoughts as well.

One of these is: the "Or" in the vocab's central query is a performance hazard. "OR" of ten is. The TeamParticipation table makes everyone a member of themselves, so the transitive-ownership check will also show all direct-ownership cases. Yet the query plan probably won't be able to avoid fetching data for the transitive check, so you may be getting one query for the price of two.

The other is: the change is pretty fundamental to the vocabulary. If the parameter you're passing is always going to be a literal, why not make this a separate vocab and use inheritance to express the commonality? It also means you don't have to kludge around the difference in query conditions.

Putting these two together, I would say: keep the current dictionary as it is. Create a separate one that's generalized for team-owned branches. It's going to be faster and easier to read, and maybe someday we'll decide that we don't actually need the direct-ownership one as much as we thought.
-- 
https://code.launchpad.net/~wallyworld/launchpad/branch-picker-team-branches/+merge/52356
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branch-picker-team-branches into lp:launchpad.