[teampractices] Code review social norms

[Subramanya Sastry]

On 03/14/2016 03:40 PM, James Forrester wrote:
> On 14 March 2016 at 19:15, Greg Grossmeier <greg at wikimedia.org 
> <mailto:greg at wikimedia.org>>wrote:
>
>     (CC'ing Matt F who lead the "Make code review not suck" session at
>     WikiDev16, not sure if his on the list or not.)
>
>     Related to the other code-review for WMF teams discussion I'd like to
>     pass along some feedback from Evan Priestley (the Phabricator lead
>     dev)
>     on how we currently do code-review in Gerrit. Specifically the
>     issue of
>     amending other people's patches.
>
>     Backgroun:
>     * This started as this task in our Phab:
>     https://phabricator.wikimedia.org/T121751 "Document best practices to
>       amend a change written by another contributor"
>     * Lot's of discussion there about what is "right" in the general
>     sense.
>     * Mukunda found out a way of making everyone happy (maybe)
>     * Mukunda proposed that solution upstream, then Evan P wrote a long
>       opinion piece on code review social contracts and basically
>     concluded
>       that our social contracts are toxic (a theme we keep hearing...)
>     ** here: https://secure.phabricator.com/T10584
>
>     I won't copy/paste it all as it's long and I don't want to lose
>     formatting, but I think it's worth while for those of us on this
>     list to
>     read and think about.
>
>
> ​I read this and have to pretty strongly disagree with the 
> characterisation given here applying to the areas with which I work. I 
> think Evan's comments are interesting, but drawing the conclusion from 
> "I think this workflow is generally toxic for new hires" but "fully 
> justified and desirable" for new volunteers into "our social contracts 
> are toxic" is not justified by the discussion or any experiences I've 
> had more recently.
>
> Indeed, I'd be more blunt: I think it's actively disrespectful to 
> peers – be they WMF staff, third party staff, or volunteers alike – to 
> *not* just tweak and merge their code. For instance, when there's a 
> typo/misplaced whitespace in a comment or it needs a quick manual 
> rebase, refusing to fix and merge isn't a great practice. People's 
> time is precious and as reviewers we should all take on the burden of 
> minor changes, and not give trivial C-1s (or Differential equivalents) 
> that push the review cycle out for another hour/week/year (depending 
> on the respondent's availability).

I am in agreement with James here. It has been common in Parsoid code 
development for reviewers to sometimes take over a patch, fix issues. 
Sometimes there is a back and forth. That sometimes seems faster and 
more efficient in terms of getting over tricky areas of code and working 
through different approaches / issues. It works out well for us because 
none of us mind that approach.

https://gerrit.wikimedia.org/r/#/c/238957/ is an example where Tim, 
Scott, and I all submitted amended patches .. and in the end, I 
acknowledged it with a Co-authored by: line in the bottom.

There are number of other instances where we have done this in the 
Parsoid code base ... to ensure quick forward progress rather than long 
drawn out review cycles.

Subbu.

>
> Looking at the contributions by recent staff and volunteers in my 
> areas over the past few months, pretty much all of them got code 
> merged in the first day or so of their starting, and pretty much all 
> of them got a minor tweak like a typo fixed for them by the 
> reviewer/merger. Helping each other out is a sign of mutual respect, 
> not disrespect.
>
> Of course, I have no idea about other areas of the code and what the 
> culture is like there (Operations, MW's API and DB stuff, /etc./). 
> Possibly this is more of an un-level ground? Certainly most of these 
> areas are places I'd fear to tread, but that's due to the endless 
> complexity and capacity for breaking things horrendously, rather than 
> dogs in the manger being toxic to new starters…
>
> J.
> -- 
> James D. Forrester
> Lead Product Manager, Editing
> Wikimedia Foundation, Inc.
>
> jforrester at wikimedia.org <mailto:jforrester at wikimedia.org> | @jdforrester
>
>
> _______________________________________________
> 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/20160314/7ea76553/attachment.html>