<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 03/14/2016 03:40 PM, James Forrester
wrote:<br>
</div>
<blockquote
cite="mid:CAEWGtDUFX=AZM+qpsg38PjnijZUD39qsn-AtnLAq8rR=jkn5Hg@mail.gmail.com"
type="cite">
<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
moz-do-not-send="true" href="mailto:greg@wikimedia.org"
target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:greg@wikimedia.org">greg@wikimedia.org</a></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 moz-do-not-send="true"
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 moz-do-not-send="true"
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>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<a class="moz-txt-link-freetext" href="https://gerrit.wikimedia.org/r/#/c/238957/">https://gerrit.wikimedia.org/r/#/c/238957/</a> 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.<br>
<br>
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.<br>
<br>
Subbu.<br>
<br>
<blockquote
cite="mid:CAEWGtDUFX=AZM+qpsg38PjnijZUD39qsn-AtnLAq8rR=jkn5Hg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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 moz-do-not-send="true"
href="mailto:jforrester@wikimedia.org" target="_blank">jforrester@wikimedia.org</a>
| @jdforrester</div>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
teampractices mailing list
<a class="moz-txt-link-abbreviated" href="mailto:teampractices@lists.wikimedia.org">teampractices@lists.wikimedia.org</a>
<a class="moz-txt-link-freetext" href="https://lists.wikimedia.org/mailman/listinfo/teampractices">https://lists.wikimedia.org/mailman/listinfo/teampractices</a>
</pre>
</blockquote>
<br>
</body>
</html>