[teampractices] Code review social norms

[Rob Lanphier]

On Tue, Mar 15, 2016 at 11:26 AM, Greg Grossmeier <greg at wikimedia.org>
wrote:

> <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.
>

Nor does brushing off my point (and James' point) by sanctimoniously
dismissing my point as snark.

You failed to bring any clarity to this conversation with this
interjection.  I'm starting to come around to Yuri's point that he made in
the T119908 conversation starting the end of December
<https://phabricator.wikimedia.org/T119908#1903835>.

I'll requote one of James' more powerful statements: "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)."  Can you please respond to this?


> 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.


Declaring this "complex and nuanced" doesn't bring clarity to it, and
doesn't address James' points.  WMF Release Engineering is expressing its
strong preference for moving from Gerrit to Differential.  You seem to be
abdicating your team's responsibility as diplomats between this user
community and the Phab upstream, and basically begging us to help you make
an argument you are having a difficult time expressing yourself.

Are you asking one of us to take over your team's job as spokespeople to
upstream?  I'll note that the upstream conversation on Upstream:T10584
<https://secure.phabricator.com/T10584> includes the following comment from
Mukunda:

> It took 6 months before I got my first patch accepted in gerrit. This is
not an exageration.
> At my previous job I deployed to production within the first week.

Greg, do you believe that upstream's tracker is the best place for us to
have that particular conversation?  I don't mind having this conversation
publicly, but it seems more appropriate for us to discuss this in a
Wikimedia venue.

Gergo and Subbu made excellent points on T10584, and I attempted to make my
point more constructively.  Can your team represent our development
community and development processes a little more charitably and
constructively?

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.


It seems a little presumptuous to expect ArchCom's support.  Do you believe
that you deserve it?

Rob
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/teampractices/attachments/20160315/4cbe77f2/attachment.html>