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

[Alan Alpert]

On Tue, Aug 13, 2013 at 12:12 PM, Oswald Buddenhagen
<oswald.buddenhagen at digia.com> wrote:
> On Tue, Aug 13, 2013 at 11:31:55AM -0700, Alan Alpert wrote:
>> On Tue, Aug 13, 2013 at 11:18 AM, Oswald Buddenhagen <oswald.buddenhagen at digia.com> wrote:
>> > On Tue, Aug 13, 2013 at 10:26:42AM -0700, Alan Alpert wrote:
>> [...]
>
>> > at this point there are no heuristics which are known to give false
>> > positives often. when you often get a -1 for something you consider ok
>> > by defintion, then it's time you rethink your position and to look for
>> > reasons.
>>
>> 3 heuristics known to give "false positives" often (where a false
>> positive means that overriding is the correct decision, often it's
>> still valuable to be informed about the potential issue, and where
>> often means that I see it maybe once a month on average).
>>
>> A) Adding nice images, which happens a lot whenever we update the QML
>> demos/examples.
>>
> that something is nice doesn't make it the right thing to do. you need
> to consider the resource trade-offs as well (including the git repos and
> the browsers of the people who will look at these images). images can be
> cropped, scaled, color-reduced. experience shows that despite various
> doc people often arguing the opposite, such optimizations often *are*
> possible without noticable detriment to the content. this is what the
> bot is for: to remind you that you should take a step back and *think*.
> so even if an override is justified, it's not a "false positive"

It is by the definition I gave, which is the definition relevant to
this conversation. If we can't use the same definitions for things,
communication may not be possible.

> , but a
> correctly placed warning sign.

And so whenever I see that warning I ask for optipng/pngcrush. And
they do. And it's still over the heuristic threshold. And then we are
correct to override.

>
>> B) Mixing WS changes, which shows up in a lot of doc rewrites/fixes
>> and is not a conceptually separate change in that context.
>>
> here it shows that you simply didn't get the point. there is no
> exception for docs or anything else. an unrelated change is an unrelated
> change, and unrelated whitespace changes happen to be so easy to spot
> that evan a stupid bot can do it. why can't you?

So you're saying that the bot's judgement call on what's unrelated is
better than mine?

Again, it looks like we have a different definition of 'unrelated'
which is hampering communication. Either write a specifically worded
"legal" document, which is reviewed by the project as a whole, to
define 'unrelated'; or allow individual approvers to use their
judgement calls knowing that not everyone thinks the same way.

> in the change you pointed to the whitespace changes all resulted from
> your editor removing trailing whitespace. maybe it's time to fix the
> problem at its root? hint: that discussion has happened on this list
> months ago already, and the problem was solved at least in qtbase (by
> bulk-removal of trailing whitespace). now it's your turn.
>
>> C) Missing license header, due to the QML auto test data. This is
>> under a separate discussion as to whether the exemption should be
>> continued.
>>
> so why exactly are you listing it as a known false positive? if
> anything, it illustrates that you bypassed the system because the bot
> pointed out something that was inconvenient to you.

That discussion started today, and until it is concluded patches are
correct to follow the existing guidelines. Which means license headers
are not required in this case.

>> >> > having an approver (i.e., somebody who
>> >> > is expected to be familiar with the coding style, commit policy, etc.)
>> >> > take the decision to override the bot is very much part of the plan.
>> >>
>> >> The point is that the approver has already decided to approve.
>> >>
>> > nonsense. they approved the code and the text. they didn't necessarily
>> > think of the subtleties of the process and the formalities of what makes
>> > a good git commit, or are among the whitespace-blind themselves. to
>> > verify sanity reports many people need to change into an entirely
>> > different mindset. so it is a *good* thing that it needs to be done
>> > separately.
>>
>> So this fits your workflow better.
>>
> actually, no. i have a 95% reliable sanity bot in my brain. i wrote the
> bot also for the remaining 5%, but mostly for other people, because they
> are apparently unable to do what is obvious to me.

And you're unable to do what's obvious to me. Am I allowed to write a
'politeness bot' which filters all your speech :P ?

Seriously though, this is not getting rid of the sanity bot or
reducing its functionality. This is solely about the workflow needed
for interacting with it.

>> I don't think the proposed change will invalidate your personal
>> workflow. Is it possible for you to imagine that other people think
>> differently?
>>
> it's actually that way *exactly because* they think differently.

Diversity should be cherished, not trampled. If everyone thought like
you then the project would be immeasurably harmed, due to the lack of
diversity alone. So we *want* people to continue to think differently.

>> > i for one *always* do sanity overrides in a separate review, simply so
>> > it's clear that the comment i write (you know that you are required to
>> > add a plausible justification, right?) relates to the sanity review.
>>
>> The case under review was one where the justification/discussion of
>> the sanity output came up immediately, was discussed specifically in
>> the comments, and was +1'd sanity review at that time. Then there were
>> several more patch sets not affecting the outcome of that discussion,
>> but they did undo the +1.
>>
> awwww ... you should have checked the box but didn't, so the contributor
> had to wait a few hours more (because apparenly no other approver was to
> be found). just terrible.

Sorry, but I care about our contributors. I want them to have the best
experience possible. I want the casual contributor who submits a one
line doc fix to feel that it was easy and fun, and come back for more.
If this helps that experience in some small way, then I think it's
worth it.

--
Alan Alpert