[teampractices] Code review social norms

[Greg Grossmeier]

<quote name="Rob Lanphier" date="2016-03-15" time="11:02:39 -0700">
> On Mon, Mar 14, 2016 at 12:40 PM, James Forrester <jforrester at wikimedia.org>
> wrote:
> 
> > On 14 March 2016 at 19:15, Greg Grossmeier <greg at wikimedia.org> wrote:
> >
> >> 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  pretty strongly disagree with Evan] 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 fully agree with this.  It troubles me to openly and strongly disagree
> with Evan, because I have so much respect for him as an upstream BDFL.  We
> have a lot to learn from him when it comes to being a healthy upstream.
> 
> That said, there is a very unhealthy contrarian attitude toward the
> competition (GitHub) which he is fostering.  If you look at the comment
> thread on T10584, you'll see some snark about GitHub's model being
> "broken".  Somehow, forcing developers to extra middleware like arc
> *isn't* broken,
> but GitHub is broken.  Riiiight.

Snark both ways doesn't help.
 
> In reading Evan's comments (and not other Phab users), his instincts seem
> to come from the Facebook new developer mentoring model, and may not be
> entirely wrong.  We need to evaluate whether we want to join him in this
> battle against the prevailing attitude popularized by GitHub.  Personally,
> I think it's a quixotic losing argument, but ultimately, WMF Release
> Engineering needs to decide where they are going to place their loyalty and
> support.

I don't think it's a matter of "placing one's loyalty" as you term it.
This isn't a war nor an election. It's a highly complex and nuanced issue
of social norms and practices that is highly dependent upon
circumstances and location. So no, I reject this characterization.

Instead, WMF Release Engineering will continue to work with all parties
to help think through code-review best practices, along with TPG and
the Architecture Committee as leaders/supporters.

Greg

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