Tweaking the code during merge is actually something differential / arcanist already supports. It's amending someone else's revision (outside of the merge process) that is currently prevented in arcanist. That's why I wrote https://secure.phabricator.com/D15468 and proposed it to upstream. I inadvertently started something much bigger than I intended.
On Mon, Mar 14, 2016 at 2:40 PM, James Forrester <[email protected]> wrote: > On 14 March 2016 at 19:15, Greg Grossmeier <[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). > > 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] | @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
