[Rpm-maint] On GitHub pull request reviews
pmatilai at laiskiainen.org
Thu Jan 12 08:22:41 UTC 2017
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
- 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
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
- Panu -
More information about the Rpm-maint