[teampractices] Code review social norms

[Greg Grossmeier]

(quick response)

Given this is going to impact our use of Phabricator (if we maintain a
local fork of arcanist or not), could reasoned replies be shared with
upstream/Evan? Thanks :)

Greg

<quote name="Subramanya Sastry" date="2016-03-14" time="15:51:19 -0400">
> 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
> 

> _______________________________________________
> teampractices mailing list
> teampractices at lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/teampractices


-- 
| Greg Grossmeier            GPG: B2FA 27B1 F7EB D327 6B8E |
| identi.ca: @greg                A18D 1138 8E47 FAC8 1C7D |