[Development] Speeding up the review process

Fri Oct 13 20:01:34 CEST 2017

Hi,

> On 13 Oct 2017, at 17:15, Frederik Gladhorn <Frederik.Gladhorn at qt.io> wrote:
> 
> Hi all,
> 
> I think this thread is valuable, let's all spin it into something positive 
> indeed. Let's discuss ways to make reviews nicer and faster indeed.
> Sorry for the top posting, I have random thoughts that I'll simply write down 
> not directly responding to any of the points.
> 
> Please just forget about point 4 of the original email and discuss the rest. 
> As long as I have any influence in how we handle Qt and contributions, we'll 
> follow open governance. I'm very sure that Lars and many others will uphold 
> the standards we have as well. So to calm everyone down: there will be no 
> preferred treatment of patches depending on employer. We've been there with 
> Gitorious, I've seen the pain first on the outside, then inside Nokia, let's 
> not repeat that, it was pure horror on both ends, open governance has made 
> everyone's work much easier.

While I am open to discussing all sorts of ideas in how we do code reviews, the one thing I will completely resist is to change our model on how you acquire merit. The only way to acquire that is by your own previous work, not by Employer or any other means.
> 
> There were some interesting comments in the thread though about personal bias 
> and meritocracy. We often know each other. I have colleagues that I know will 
> mostly produce patches that are good and will be around to deal with flaws in 
> them, they do get my personal bias towards an easier +2. So do maintainers of 
> a module and other people that earned a standing with me and that I expect to 
> be around. Patches from people that I've never heard of before get more 
> scrutiny. I think we all (more or less consciously) act that way and that's 
> fine. There are people in the community that I know better than in The Qt 
> Company, they also get a bias from me.

+1. That is one part of what merit is all about. 

> So if Viktor works intensely with people in the same office, it should be easy 
> to get approvals by them. In fact that's the way I see people progressing on 
> their work in Qt quickest: they team up for an area and push each others' 
> patches forward with mutual review. If you want to make fast progress, get to 
> know others in the Qt community and team up with them, help each other and 
> review each others' patches, knowing where to look more careful and when to be 
> relaxed.
> 
> I've been pondering this for a while and it's not fair. And that's actually 
> fine. Bias and merit is OK. We're not a democracy.
> 
> That does not mean we should ignore anyone or be disrespectful. I expect 
> everyone independent of employer to be positive and friendly in reviews.
> Reviews are sadly negative by nature, but please assume good intentions and 
> try to phrase thing positively, ask questions and make the process work for 
> everyone.

Reviews are what help all of us write better code, and there to ensure that changes improve our code base and are aligned with where we want to go. The exception are probably things where we could leave the reviewing, criticising and fixing to tools (like e.g. code formatting).
> 
> I have personally changed to sometimes giving a +2 and still criticizing minor 
> things (a typo in the commit message or so). Usually people ended up fixing 
> those even though they could have clicked the stage button, so I think we can 
> sometimes be more relaxed when it comes to minor details.
> I've become convinced that perfection is the enemy of the good - if a patch 
> makes things only better, but it could be even better, then consider writing 
> the follow up patch yourself. Of course it's still OK to point out optional 
> improvements, but they don't need to all happen in the same patch.

Agree. We sometimes block things because they improve but aren't perfect yet. In many cases it's still better to get the improvement in, because the contributor might not have the time or energy to make it perfect at this point in time.
> 
> Collaboration:
> I wish gerrit made it easier. I've experimented with handing patches over to 
> other people, I would encourage that as well. If you know you won't really 
> finish the thing, but someone is interested in it, instead of letting them 
> review it, if they care more than yourself, make it clear that the patch is up 
> for grabs, to be adopted, improved upon. That's not super easy in gerrit from 
> a social perspective, there seems to be clear ownership and randomly pushing 
> over other's patches is not a sensible thing to do of course, so I think the 
> authors need to make it clear to others that they're fine with changes to their 
> patches.
> 
> Cheers,
> Frederik
> 
> On fredag 13. oktober 2017 13.04.46 CEST Viktor Engelmann wrote:
>> 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:
>>     1. when you only changed minor things like a variable name (except
>>        in public API), a string, a comment or qdoc entry
>>     2. 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 )
>>     3. 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.
>>     4. or more general: when you only made an autotest harder to pass -
>>        like adding a Q_VERIFY, Q_COMPARE, etc.
>>     5. when the change is something auto-generated - like you just
>>        updated plugins.qmltypes using qmlplugindump
>>     6. when you only changed something in accordance to the guidelines
>>        - like Q_DECL_OVERRIDE -> override
>>     7. 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.
> 
> 
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development