[teampractices] Code review social norms

[Mukunda Modell]

Tweaking the code during merge is actually something differential /
arcanist already supports. It's amending someone else's revision (outside
of the merge process) that is currently prevented in arcanist. That's why I
wrote https://secure.phabricator.com/D15468 and proposed it to upstream. I
inadvertently started something much bigger than I intended.

On Mon, Mar 14, 2016 at 2:40 PM, James Forrester <jforrester at wikimedia.org>
wrote:

> 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
>
> _______________________________________________
> 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/20160315/c696bdec/attachment.html>