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

[Oswald Buddenhagen]

On Tue, Aug 13, 2013 at 12:34:19PM -0700, Alan Alpert wrote:
> On Tue, Aug 13, 2013 at 12:12 PM, Oswald Buddenhagen <oswald.buddenhagen at digia.com> wrote:
> > 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.
> 
you should always optipng, not just when the bot complains. ;)
anyway, as i said, there are other ways to optimize, by changing the
content to be less resource-hungry. it is _usually_ possible to do that
without a noticable trade-off if one is just willing to consider it.

> >> 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?
> 
finally you are getting it. ;)

> 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.
> 
the definion of "unrelated" follows directly from the wording in the
commit policy: if you read the summary line in the "git blame" output
and after triple-checking you still think "WTF does that have to do with
this LOC?!", you found something unrelated.

and unless the summary says "fix coding style" or "remove trailing
whitespace", respective changes are rather obviously "wtf?!".

and before you start arguing that again, read the policy again, and try
to be cooperative for a change, rather than trying to find loopholes to
exploit.

> >> 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.
> 
it's *still* not a "known false positive".

> >> 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.
> 
the whole point of my explanation was to point out that the bot is there
to avoid stupid mistakes *despite* the diversity.

> > 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.
>
as it happens, i totally agree.

> If this helps that experience in some small way, then I think it's
> worth it.
> 
nonsense.
any kind of security/safety mechanism causes some friction, so by
definition your idea of improvement would mean abolishing any kind of
restrictions. the reality is that it is about trade-offs. having a
contributor wait a few hours because the reviewer did a stupid mistake
is a *perfectly* acceptable trade-off to uninformed contributors
mindlessly overriding the bot just because they can.