On 03/14/2016 03:40 PM, James Forrester wrote:
On 14 March 2016 at 19:15, Greg Grossmeier <[email protected]
<mailto:[email protected]>>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.
[email protected] <mailto:[email protected]> | @jdforrester
_______________________________________________
teampractices mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/teampractices
_______________________________________________
teampractices mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/teampractices
