<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">On 14 March 2016 at 19:15, Greg Grossmeier </span><span dir="ltr" style="font-family:arial,sans-serif"><<a href="mailto:greg@wikimedia.org" target="_blank">greg@wikimedia.org</a>></span><span style="font-family:arial,sans-serif"> wrote:</span><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">(CC'ing Matt F who lead the "Make code review not suck" session at<br>
WikiDev16, not sure if his on the list or not.)<br>
<br>
Related to the other code-review for WMF teams discussion I'd like to<br>
pass along some feedback from Evan Priestley (the Phabricator lead dev)<br>
on how we currently do code-review in Gerrit. Specifically the issue of<br>
amending other people's patches.<br>
<br>
Backgroun:<br>
* This started as this task in our Phab:<br>
  <a href="https://phabricator.wikimedia.org/T121751" rel="noreferrer" target="_blank">https://phabricator.wikimedia.org/T121751</a> "Document best practices to<br>
  amend a change written by another contributor"<br>
* Lot's of discussion there about what is "right" in the general sense.<br>
* Mukunda found out a way of making everyone happy (maybe)<br>
* Mukunda proposed that solution upstream, then Evan P wrote a long<br>
  opinion piece on code review social contracts and basically concluded<br>
  that our social contracts are toxic (a theme we keep hearing...)<br>
** here: <a href="https://secure.phabricator.com/T10584" rel="noreferrer" target="_blank">https://secure.phabricator.com/T10584</a><br>
<br>
I won't copy/paste it all as it's long and I don't want to lose<br>
formatting, but I think it's worth while for those of us on this list to<br>
read and think about.<br></blockquote><div><br></div><div><div class="gmail_default" style=""><font face="arial, helvetica, sans-serif">​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 "</font><span style="font-family:arial,sans-serif">our social contracts are toxic</span><font face="arial, helvetica, sans-serif">" is not justified by the discussion or any experiences I've had more recently.</font></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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 <b>not</b> 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).</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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, <i>etc.</i>). 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…</div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">J.</div></div>-- <br><div class="gmail_signature">James D. Forrester<br>Lead Product Manager, Editing<br>Wikimedia Foundation, Inc.<br><br><a href="mailto:jforrester@wikimedia.org" target="_blank">jforrester@wikimedia.org</a> | @jdforrester</div>
</div></div>