[Rpm-maint] On GitHub pull request reviews

[Panu Matilainen]

Folks,

Just this week I've wasted considerable amount of time with pull 
requests that clearly had never been tested at all (neither by the 
submitter nor the reviewers) because they were completely broken: 
compiler warnings, segfaults, written against an imaginary library (!), 
referring to non-existent variables. Yet all had several "looks good to 
me" style comments and all/most actually also marked as approved.

This is not a contest where the the reviewer with most approved PRs wins 
the Most Positive Reviewer prize at the end of the year. I appreciate 
the enthusiasm to help with the reviews, I really do, but speed and 
quantity are not desired values there. Quality is. Everybody's time is 
limited, which is all the more reason to make the time used for 
something actually count. If you decide to use your precious time on rpm 
patch/PR review then here are some guidelines to make it count:

1) A full review consists of numerous steps that can be processed 
relatively independently:
    - does the case/idea seem valid?
    - does the code look technically correct?
    - does the code conform to rpm coding style?
    - does the patch apply and compile cleanly on rpm git master?
    - does it actually work on rpm git master?

2) When commenting/reviewing, be explicit about what you did or didn't 
do. If you like the idea, but didn't look at the code or only briefly 
skimmed it, say so. If the code looks clean but you're not convinced 
about the idea, say it. If you tested the functionality but don't 
understand the code, say so. Etc. All that is valuable information and 
can save others from duplicating that work! But stating "looks good to 
me" clearly doesn't mean sh** so it is not helpful at all - to the 
contrary in fact.

3) Don't flag a PR as approved unless you *really* understood the code 
and the case, compiled AND functionality tested it and are absolutely 
positive it should be included. And even then it might be better not to, 
because it's likely to send the submitter home patting him/herself on 
the back on job well done on the way back when another reviewer might 
spot further issues needing addressing. The only truly relevant approval 
of a patch is getting merged.

4) Quality over quantity. Really. Rather spend your time on one 
thorough, thoughtful review than hastily skimming through three 
different ones.

5) Everybody accidentally submits the wrong version sometimes, and no 
amount of review will catch all mistakes, bugs and regressions and 
that's perfectly okay, we're all just humans here (I hope!). But what 
really, really drives me off the wall is patches that are *obviously* 
entirely untested. If you spot such a patch then please do flag it as 
such so that others don't waste their time on it. If the submitter 
cannot be bothered to test their stuff then maybe it doesn't deserve our 
attention either.

	- Panu -