[Development] Proposal: Allow contributors to +1 sanity review.

[Knoll Lars]

Hi,

Catching up on this long thread finally. I am actually with Andre (and
others arguing similarly) here. The sanity bot is a good thing to have,
and it finds issues a lot more often then making mistakes.

Yes, there are a few things, where the bot can be wrong and for that we
can overrule it. But I also think that this is an approver's job to do,
not a job for everybody. An approver is supposed to know our commit and
coding style guidelines and can then consciously decide that the commit is
ok. This is something we don't expect a contributor that's new to the
project to know.

And I don't believe this is actually making it harder to get new
contributors on board. Actually I believe it makes it easier for new
people, as they have a tool that helps them follow our coding standards
and guidelines. Without it the turnaround time for fixing coding style
errors would often be a lot longer as you'd have to wait for a manual
review pointing those out. And it also makes the approvers life easier, as
he doesn't have to worry as much about these things and point out mistakes
the bot already found.

So I don't see a reason to change the status quo.

Cheers,
Lars


On 15.08.13 22:06, "André Pönitz"
<andre.poenitz at mathematik.tu-chemnitz.de> wrote:

>On Thu, Aug 15, 2013 at 11:44:31AM -0700, Thiago Macieira wrote:
>> On quinta-feira, 15 de agosto de 2013 18:32:09, Oswald Buddenhagen
>>wrote:
>> > ironically, you failed to respond to exactly the part of the message
>> > that you quoted. yes, sometimes one forgets the sanity override. *what
>> > is the actual, observable harm?*
>> 
>> A full 24-hour cycle because the approver forgot to give the necessary
>>+1 
>> sanity check. I didn't think I needed to reply because Alan had already
>>said 
>> that.
>
>It should be rather quick to get another approver's +1 on a _really_
>_accidentally_ missing sanity-bot override by simply asking on #qt-labs.
>
>I don't dispute that it might _theoretically_ happen that #qt-labs is not
>manned by half a dozen approvers at 5 am on Christmas Day, but to incur a
>real delay this has to coincide with a false positve from the bot _and_
>with the original approver forgetting to override it at the same time.
>
>> You dismissed the argument. But that's still my argument and I don't
>>dismiss 
>> it (in other words, I think you're wrong to dismiss it).
>
>The argument is valid, but it has to be weighted with the likelihood that
>the situation occurs. That likelihood is very small. I've yet to see
>someone waiting for a (justified) sanity-bot override for 24 hours.
>
>> Approvers forget to give the +1 to sanity because a reviewer is not
>>supposed 
>> to be looking at the sanity -- that's the contributor's job.
>
>"Sanity" covers a lot of aspects, lots of rules, written and unwritten. It
>is not to be expected that a first-time contributor can take all of them
>into account, it is even less expected that he can clearly recognize a
>wrong -1 from the bot as inappropriate.
>
>> That's why we have a bot in the first place. The reviewer will review
>>the
>> code and give a code check rating and will usually not look at the
>> sanity.
>
>A -1 from the bot is not hard to miss in a review, and should trigger
>double-checking. I understand one might forget to tick it off after an
>hour-long review, but I really doubt this happens regularly.
>
>> Therefore, it's very common to forget to give the +1 sanity.
>
>How often did that happen to you personally, and how much was the delay
>caused by that?
>
>> You said that Git permits highly parallel work. Well, yes, but there's
>>still a 
>> lot of serial work. In the worst case scenario, the missing sanity
>>means a 
>> commit misses the merge-to-dev window and the contributor will need to
>>wait a 
>> week for the next one so they can rebase some dev work.
>
>> [Big snip]
>> 
>> Allowing approvers only to override means inattentive non-approvers
>>can't 
>> override the bot. That says nothing about the inattentive approvers,
>>who 
>> submit 95% of the code contributions anyway. But it means a portion of
>>those 
>> 5% of the contributions may end up delayed.
>
>There is way more work _that passed sanity review_ that misses merges like
>that. This is optimizing for the wrong case.
>
>> Allowing non-approvers to override means we could have inattentive or
>> inexperienced contributors bypassing the sanity rules. Since the
>>contribution 
>> *still* needs to be approved by an approver, there's an opportunity to
>>catch 
>> the mistake. If the mistake isn't caught, it's just that: a mistake.
>>We're all 
>> human.
>
>The contribution _includes_ sanity. If the approver does not approve
>sanity, he does not approve the contribution.
>
>Approvers can and do err, but that could as well happen in the "regular"
>part of the review. If you go the full mile here, we would need to allow
>contributors tick +2 on the "real" review themselves, after all, the
>reviewer might just have forgotten that...
>
>Andre'
>
>_______________________________________________
>Development mailing list
>Development at qt-project.org
>http://lists.qt-project.org/mailman/listinfo/development