[teampractices] Code review social norms

[Kevin Smith]

Ah. I suspect another factor in my perspective is that I am very open to
pair programming. Often (as the reviewer) I would get on the phone with the
author and work through some code to the point that we were both aligned.
The author could then fix their code, and I could re-review it.

The case when the original author is asleep or on vacation, and the patch
needs to be pushed through, is different. I know it happens, but I wonder
if the norms were set up to discourage modifying someone else's patch, how
often it would be worth breaking those norms. In other words, how often are
people modifying patches because it's normal to do so, versus because it is
actually helpful and/or necessary.

In case it's not clear, I'm not that familiar with development at the WMF.
Every environment is different, but I'm just sharing what worked (really
well) for me somewhere else. Some may apply here, and I'm sure some won't.
I am intentionally trying to poke at some existing conventions and biases;
if they stand up to some scrutiny, great, but hopefully folks are open to
re-thinking them when it makes sense. Maybe we don't have to cut the ends
off our pot roasts[1].

[1] http://selfdefinedleadership.com/blog/?p=158



Kevin Smith
Agile Coach, Wikimedia Foundation


On Wed, Mar 16, 2016 at 11:00 AM, Subramanya Sastry <ssastry at wikimedia.org>
wrote:

> On 03/16/2016 01:33 PM, Kevin Smith wrote:
>
>> I'm curious what the inherent benefit is of having multiple people
>> collaborate on a patch, as opposed to having a series of patches by
>> different people, each of which advances the product incrementally. At
>> least, it sounded like you (Rob) were advocating collaboration for its own
>> sake.
>>
>
> Here are some numbers from the Parsoid codebase to show patches that had
> an explicit co-author tag. This doesn't account for other patches where
> someone other than the author tweaked the patch slightly (that doesn't
> quite count as co-authoring) and merged them.
>
> [subbu at earth bin] git log --oneline --no-merges | wc
>    5465   50651  349772
> [subbu at earth bin] git log | grep -i Co-author | wc
>      18      82    1013
>
> Not a whole lot, but a non-trivial number of patches.
>
> Here are two patches that had the co-author tag:
> * https://gerrit.wikimedia.org/r/#/c/238957/
> * https://gerrit.wikimedia.org/r/#/c/181177/
>
> Both were tricky pieces of code to get right. Sometimes, it is hard to
> communicate a fix / problem in a patch via review comments. It is easier to
> write the code and show. Sometimes, that is simply the best way to address
> a problem. Sometimes, the original author is lost in some other project /
> is on vacation / ... whatever ... and patch needs to be pushed forward.
>
> Subbu.
>
>
>
> _______________________________________________
> teampractices mailing list
> teampractices at lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/teampractices
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/teampractices/attachments/20160316/dcbf1b3f/attachment.html>