[Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

Adam Treat adam.treat at qt.io
Fri Oct 13 21:17:17 CEST 2017


+1

I also really appreciate when a reviewer is up front about the steps 
necessary to get the patch over the finish line. And if you are just 
doing a drive-by review pointing out all the mistakes, but not willing 
to +2 in the end then please state that up front as well. To be clear, 
I'm not against drive-by reviews where the reviewers never intend to +2 
in the end (as they can still help me improve my code!!), but I just 
would like them to be up front about this.

Cheers,
Adam

On 10/13/2017 10:56 AM, Thiago Macieira wrote:
> On Friday, 13 October 2017 04:04:46 PDT Viktor Engelmann wrote:
>>   5. Set a deadline for criticism on the general approach to a change.
>>      Too often I have had the situation that I uploaded a patch, then we
>>      debated the qdoc entries, variable names, method names, etc FOR
>>      MONTHS - and when I thought we were finally wrapping up and I could
>>      soon submit it, someone else chimes in and says "this should be done
>>      completely differently". Even if the person is right - they should
>>      have said that months earlier - before I wasted all that valuable
>>      time on variable names, compliance with qdoc guidelines, etc.
>>      In earlier discussions I have been told that such a deadline would
>>      have to be long, because someone who might have an opinion might be
>>      on vacation. IMHO, this doesn't count, because a) you can still make
>>      an exception to the rule in such a situation and b) by that logic
>>      you should never approve anything, because we also might hire a new
>>      employee in 10 years who might have an opinion.
> I urge all reviewers to adopt the three-phase review process outlined in Sarah
> Sharp's blog:
>
> 	http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/
>
> The only problem with this process is that different reviewers will reach
> different stages at different times. So some of them may already have finished
> the review of the concept and may be already providing criticism on the
> implementation, while others are still not convinced of the concept.
>
> Therefore, I add this extra advice on top of the blog:
>
> When you're a reviewer in a change with multiple other reviewers, try to keep
> pace with the other reviewers and not race ahead. If it's not evident where
> the other reviewers are, communication is good. Post something like
>
> "Thank you for your change, I like the idea and I think you implemented it
> properly. I have some comments on your coding style, but we'll discuss that
> when other reviewers have had a chance to review your implementation too."
>




More information about the Development mailing list