[teampractices] Code review social norms

[James Forrester]

On 14 March 2016 at 19:15, Greg Grossmeier <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).

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 | @jdforrester
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/teampractices/attachments/20160314/2eff4449/attachment.html>