[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