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

[Martin Smith]

+1

I am a member of the Qt documentation team, and I am often included as a reviewer for code changes that also include changes to qdoc comments. I always assume I am meant to review only the documentation, so if the documentation is ok, I give the change a +1 and add a comment that I only reviewed the documentation.

Is this the right way to do it? Maybe it should be formalized in the system.

martin
________________________________________
From: Development <development-bounces+martin.smith=qt.io at qt-project.org> on behalf of Viktor Engelmann <viktor.engelmann at qt.io>
Sent: Friday, October 13, 2017 1:04:46 PM
To: development at qt-project.org
Subject: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

On the [Interest] mailing list there was a discussion about the review-process taking to long and we also had multiple discussions about that at the world summit. I have complained about this myself, so I would like to start a new thread and collect your thoughts and ideas on how to improve the situation.

My suggestions would be

  1.  Allow approving your own commits under certain circumstances. examples:
     *   when you only changed minor things like a variable name (except in public API), a string, a comment or qdoc entry
     *   when you already had a +2 and only changed minor things like a variable name, a string, a comment or qdoc entry (or more generally: when you already had a +2 and only did something that is also on this list :-D )
     *   when you only increased a timeout in an autotest. Increasing timeouts is a safe change - it can only solve false negatives and false positives, but not create false positives or false negatives.
     *   or more general: when you only made an autotest harder to pass - like adding a Q_VERIFY, Q_COMPARE, etc.
     *   when the change is something auto-generated - like you just updated plugins.qmltypes using qmlplugindump
     *   when you only changed something in accordance to the guidelines - like Q_DECL_OVERRIDE -> override
     *   when you have a certain count of +1's from people who have approver rights
  2.  Towards that last point: I think many of us are afraid to get blamed for +2'ing something that causes problems later (introduces a new bug or so), but as far as I have seen, nobody gets blamed for such problems, so we should not be THAT afraid of approving something. Also, don't forget that there is still the CI to get past!
  3.  Remember that brain-cycles are far more expensive than CPU cycles - so when in doubt, rather test-run something on the CI than make a human think about whether the CI "would" accept it. If that causes CI outages, we need to buy more CI machines. It is just a naive fallacy to "save" CI expenses by assigning the CI's work to employees who are much more expensive.
  4.  I don't think we need to be as paranoid towards contributions from our own employees as we need to be towards external contributions.
  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.


--

Viktor Engelmann
Software Engineer

The Qt Company GmbH
Rudower Chaussee 13
D-12489 Berlin
Viktor.Engelmann at qt.io<mailto:Viktor.Engelmann at qt.io>
+49 151 26784521
http://qt.io

Geschäftsführer: Mika Pälsi,
Juha Varelius, Mika Harjuaho
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht
Charlottenburg, HRB 144331 B

The Future is Written with Qt
www.qtworldsummit.com<http://www.qtworldsummit.com>