Re: [teampractices] Code review social norms

Ah. I suspect another factor in my perspective is that I am very open to pair programming. Often (as the reviewer) I would get on the phone with the author and work through some code to the point that we were both aligned. The author could then fix their code, and I could re-review it. The case

Re: [teampractices] Code review social norms

On Wed, Mar 16, 2016 at 4:25 AM, Mukunda Modell wrote: > 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. >

Re: [teampractices] Code review social norms

Le 18/03/2016 01:47, Gergo Tisza a écrit : > > Nontrivial changes in a merge commit is a bad practice, as the changes > are invisible to git diff / git log -p unless you explicitly tell git to > diff to the non-default parent, so they inevitably cause confusion. Hello, It is slightly more

Re: [teampractices] Code review social norms

On Wed, Mar 16, 2016 at 12:48 AM, Kevin Smith wrote: > I would mention that in some cases, I would prefer to accept the commit as > is, and then perform minor refactoring, such as changing a name, fixing a > typo, or rearranging the code. Not only does that clearly separate

Re: [teampractices] Code review social norms

On Wed, Mar 16, 2016 at 6:33 PM, Kevin Smith wrote: > I'm curious what the inherent benefit is of having multiple people > collaborate on a patch, as opposed to having a series of patches by > different people, each of which advances the product incrementally. > I don't

Re: [teampractices] Code review social norms

On Thu, Mar 17, 2016 at 6:08 PM, Gergo Tisza wrote: > some large changes simply cannot be broken up into independent small ones > due to circular dependencies, unless you want to leave the code in a broken > state between commits (in which case you lose most of the

Re: [teampractices] Code review social norms

On 03/16/2016 01:33 PM, Kevin Smith wrote: I'm curious what the inherent benefit is of having multiple people collaborate on a patch, as opposed to having a series of patches by different people, each of which advances the product incrementally. At least, it sounded like you (Rob) were

Re: [teampractices] Code review social norms

On 16 March 2016 at 10:33, Kevin Smith wrote: > > I wonder if my heavy Test-Driven Development and heavy Refactoring > preferences are what is putting me on the opposite side of this debate. I > have no problem with a commit followed immediately by a rename commit > followed

Re: [teampractices] Code review social norms

Le 16/03/2016 00:48, Kevin Smith a écrit : > There are lots of different cases here, as well as existing norms, and > also preferences. > > I would mention that in some cases, I would prefer to accept the commit > as is, and then perform minor refactoring, such as changing a name, > fixing a

Re: [teampractices] Code review social norms

On 03/16/2016 02:33 AM, Rob Lanphier wrote: On Tue, Mar 15, 2016 at 8:03 PM, Mukunda Modell > wrote: On Tue, Mar 15, 2016 at 6:48 PM, Kevin Smith > wrote: I would mention

Re: [teampractices] Code review social norms

On Tue, Mar 15, 2016 at 8:03 PM, Mukunda Modell wrote: > On Tue, Mar 15, 2016 at 6:48 PM, Kevin Smith wrote: > >> I would mention that in some cases, I would prefer to accept the commit >> as is, and then perform minor refactoring, such as changing a

Re: [teampractices] Code review social norms

On Tue, Mar 15, 2016 at 6:48 PM, Kevin Smith wrote: > > I would mention that in some cases, I would prefer to accept the commit as > is, and then perform minor refactoring, such as changing a name, fixing a > typo, or rearranging the code. Not only does that clearly

Re: [teampractices] Code review social norms

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

Re: [teampractices] Code review social norms

On Tue, Mar 15, 2016 at 11:26 AM, Greg Grossmeier wrote: > > > On Mon, Mar 14, 2016 at 12:40 PM, James Forrester < > jforres...@wikimedia.org> > > wrote: > > > > > On 14 March 2016 at 19:15, Greg Grossmeier wrote: > > > > > >> Backgroun: > > >> * This

Re: [teampractices] Code review social norms

There are lots of different cases here, as well as existing norms, and also preferences. I would mention that in some cases, I would prefer to accept the commit as is, and then perform minor refactoring, such as changing a name, fixing a typo, or rearranging the code. Not only does that clearly

Re: [teampractices] Code review social norms

> On Mon, Mar 14, 2016 at 12:40 PM, James Forrester > wrote: > > > On 14 March 2016 at 19:15, Greg Grossmeier wrote: > > > >> Backgroun: > >> * This started as this task in our Phab: > >> https://phabricator.wikimedia.org/T121751 "Document best

Re: [teampractices] Code review social norms

(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 > On 03/14/2016 03:40 PM, James Forrester wrote: > >On 14 March 2016 at 19:15, Greg Grossmeier

Re: [teampractices] Code review social norms

On 03/14/2016 03:40 PM, James Forrester wrote: On 14 March 2016 at 19:15, Greg Grossmeier >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

Re: [teampractices] Code review social norms

> (CC'ing Matt F who lead the "Make code review not suck" session at > WikiDev16, not sure if his on the list or not.) Gah, it was Brian not Matt. Apologies. Correcting CC. Brian, see below: > Related to the other code-review for WMF teams discussion I'd like to > pass along some feedback from

[teampractices] Code review social norms

(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