[teampractices] Code review

[Matthew Flaschen]

On 06/08/2015 04:23 AM, Quim Gil wrote:
> Thank you, very useful. I'm happy to see that our problem with code
> review keeps becoming more evident, if only as a step to finally doing
> something about it.
>
> http://korma.wmflabs.org/browser/gerrit_review_queue.html shows that our
> backlog of patches waiting for review keeps growing. 1542 last month,
> was 1033 a year ago. If you consider the time and skills it takes to
> produce one patch, you may imagine the big black hole of frustration and
> waste of real money we have there.
>
> While comparing satisfaction across teams is subjective and complex,
> comparing code review waiting times between WMF teams can be measured
> objectively.

If we really want to incentivize this, I suggest we discuss these 
metrics in team quarterly reviews (and perhaps monthly too).

Note that these metrics should not just be reviews to the team-developed 
extensions, but also reviews by team members of volunteer patches to 
other repositories.

There should also be an understanding that properly reviewing a patch 
can sometimes take far longer that developing the patch.

This means it can sometimes result in a slight change in velocity for 
the team (remember, velocity is both speed and direction), which also 
has to be understood at quarterly review time.

Finally, sometimes it's appropriate to decline a patch even if it's not 
actively harmful (or it's unknown whether it is).  The Phabricator 
upstream project is a good example of this, though I don't know if we 
need to decline as many as they do.  However, they are certainly a good 
example about the importance of checking whether a non-trivial patch 
would be accepted before starting development work.

Matt Flaschen