[teampractices] Code review social norms

Wed Mar 16 13:27:20 UTC 2016

On 03/16/2016 02:33 AM, Rob Lanphier wrote:
> On Tue, Mar 15, 2016 at 8:03 PM, Mukunda Modell <mmodell at wikimedia.org 
> <mailto:mmodell at wikimedia.org>> wrote:
>
>     On Tue, Mar 15, 2016 at 6:48 PM, Kevin Smith <ksmith at wikimedia.org
>     <mailto:ksmith at wikimedia.org>> 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 authorship, but it would also
>         encourage those changes to be reviewed by someone other than
>         that author.
>

This works when CI jobs aren't red. When CI jobs are red (for example, 
jscs / jshint style guide for javascript which would count as minor 
tweaking), it might sometimes be faster for the reviewer to fix them and 
merge them.

>     This ^
>
>     I think this says what I've been trying to say, only better.
>
>
> Thank you Kevin and Mukunda.  I think I still probably disagree with 
> you, but I understand what you're trying to say a lot better now, and 
> I'm now in the "mild disagreement" category.
>
> My mild disagreement: I think it's good to have a system where people 
> collaborate on a patch before it lands in trunk/mainline.  Subbu's 
> case seems reasonable to me.

To be clear, for sure, we can find other ways of collaboration and other 
ways of fixing / amending patches.

But, overall, I haven't understood why the tool has to be so opinionated 
about this. It seems more flexibility is better .. having a flag letting 
projects turn on/off this feature seems a better approach rather than 
dictate workflows for all users / projects?

Subbu.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/teampractices/attachments/20160316/ae530700/attachment.html>