+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."
_______________________________________________
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
