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

[Oswald Buddenhagen]

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:
> >> We *define* the system. The fact that I have to work around it in
> >> order to do my job properly shows that the system is defective and
> >> needs to be rectified.
> >>
> > or it means that you are not doing your job right, and are trying to
> > find loopholes to outsmart the system.
> 
> If the bot can do my job better than I can, why don't we let it write
> the code for us :P ?
> 
nice straw man.
the bot is a critic. it doesn't have to be better at writing code, only
at pointing out your mistakes.

> The "system" in question is a script that you wrote. Based a a wiki
> page which I believe you wrote (some history seems lost). Is some
> outside scrutiny allowed, or can we not even question the "system"?
> 
the script is public and open for merge requests.
what you need to do is to make a convincing point that the system needs
fixing and that your proposed fix is correct.
what you are not allowed to do is to bypass the system solely on grounds
that you think it's broken.

> > 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", but a
correctly placed warning sign.

> 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?

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.

> >> > 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.

> 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.

> > 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.