[Development] [RFC] more gerrit codereview scores?

Knoll Lars Lars.Knoll at theqtcompany.com
Mon Mar 9 08:30:33 CET 2015


On 07/03/15 04:03, "Thiago Macieira" <thiago.macieira at intel.com> wrote:

>On Friday 06 March 2015 17:42:00 Oswald Buddenhagen wrote:
>> 1) i'd like to propose the introduction of the code review score -3.
>
>> -1: "I would prefer this is not merged as is", advisory, non-sticky
>> -2: "This shall not be merged as is", blocking, non-sticky
>> -3: "This shall not be merged [at all]", blocking, sticky
>
>Agreed, this makes sense. The -1 means "if someone approves, ignore me",
>whereas -2 means "this needs to change, can't be merged".
>
>The -3 is just to indicate an approach that can never work, such as a
>feature 
>we cannot accept or a patch submitted to the wrong branch. -2 are already
>rather rare, so I expect -3 to be used only once in a blue moon.

Agree, it would be nice to have a non sticky -2, so the sticky -3 makes
sense.
>
>> 2) i'd like to propose the introduction of the code review score +3.
>> 
>> let's start with the scores:
>> 
>> +3: "Looks good to me, approved", enabling
>> +2: "Looks good to me, but someone else must approve", advisory
>> +1: "Someone else must review this", advisory
>> 
>> possible uses:
>> - non-approvers (specifically, not-yet-approvers) would have two levels
>>   to express their opinion
>
>You mean four levels, since they also get -1 and 0.
>
>> - the new +1 gives the possibility to explicitly give a neutral score
>>   (substitute for +0, which gerrit does not permit)
>> - *maybe* some approvers would feel less inclined to approve changes
>>   they don't fully understand (yes, this is actually a problem), simply
>>   because of the psychological effect of the possibility to express the
>>   opinion with more "numerical nuance".
>> 
>> i don't feel very strongly about this one, but i think it would add
>> value.
>
>I don't like this one.
>
>If you don't want to express an opinion, leave your score at 0. I don't
>see 
>the need to have a value saying "I've reviewed but have no opinion". I
>also 
>don't see why approvers are giving +2 if they don't fully understand it.
>Not 
>only should they be using the right value for that, this change wouldn't
>help 
>in any way since they could just turn around and give +3 to changes they
>don't 
>fully understand.
>
>As a drawback, it would make Qt's Gerrit behave very differently from
>everyone 
>else, where a +2 does mean approval.
>
>In my opinion, this change has no pros and has cons.

While I see the reasoning behind, I think this overcomplicates things. A
non-sticky -2 that balances a +2 should be enough to solve most of the
issues, and the proposed +1 sounds very much like the current +0 to me, so
we can IMO just as well leave it out.

Cheers,
Lars



More information about the Development mailing list