(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 <g...@wikimedia.org > ><mailto:g...@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. > > > >jforres...@wikimedia.org <mailto:jforres...@wikimedia.org> | @jdforrester > > > > > >_______________________________________________ > >teampractices mailing list > >teampractices@lists.wikimedia.org > >https://lists.wikimedia.org/mailman/listinfo/teampractices > > _______________________________________________ > teampractices mailing list > teampractices@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 | _______________________________________________ teampractices mailing list teampractices@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/teampractices