Just a comment before this discussion gets entirely side tracked.
On Mon, Oct 12, 2015, at 11:45 CDT, Ian Delaney <del...@iinet.com.au> wrote:
> [...]
> Users are neither seasoned nor prepared for the type of review put
> upon them by him and mgorny.
My impression is that the reception of the code review on github is
predominantly positive. The point is - we _have_ to do some sort of code
review prior to merging (or stick to the old "let a developer do all the
work" workflow we suffered from all the years).
A very nice example of how this can be facilitated to get a lot of user
contributions is the science overlay - and I think the current review
for gentoo/gentoo is not far away from the approach, word choice, or
debate culture we have there.
> These users still needed support and a voice to help them speak up, and
> they did. Still, these members have fashioned themselves to teach and
> service users, They have alot of adapting to do before users will
> embrace their attempts to educate them.
Seriously? Shall we now all take an online tutorial in order to be
qualified to "help out"? Or shall we just merge every pull-request in
order to not have to interact with users?
There are some observations for Julian and Michał (as the two primary
reviewer on github) that I have, though. I think it might be worthwhile
to think about it and maybe "codify" it in the reviewer best practices
(if not already present):
- I suggest that only one reviewer administers a given pull-request at
a time. I have seen two instances where first Julian had roughly 20
remarks and after those were resolved Michał slabbed another 10 code
remarks on top of it.
This seems to be a bit over the top. It feels a bit like vultures
tearing apart dead prey ;-)
The bottom line should be that if one Gentoo developer is satisfied
with the state of a pull-request it should be okay.
- With respect to the current post- "peer review" of commits: I suggest
that comments on coding style (if not violating our indent rules) and
other personal choice is kept at a bare minimum (or avoided
entirely). While I appreciate comments on factual errors I have
commited [1], I totally hate discussing something like whether a "\"
might be omitted in bash or not.
- Further, "weird" or "unusual" choices of, e.g., how to form up a
configuration variable might be due to historical (and sometimes
political reasons). For example, because of a switch of
maintainership, or due to a developer just "helping out" (and
obviously avoiding invasive, large changes).
Thus, comments like "this is hard to read", "why do you do it like
that" are usually more annoying than helpful [2].
- And last, keep in mind that discussing the current state of an ebuild
on a version bump is equivalent to discussing an arbitrary amount of
commits over the last years (and given the vivid history of some
ebuilds of code fragments from a lot of people).
An example here would be the missing "|| die" statements on a sed
statement that manipulated an entirely gentoo-developer controlled
configuration file [3]. While it is technically correct that a "die"
is missing - it doesn't hurt terribly much either (no file involved
is controlled by upstream and might silently change during a version
bump).
Best,
Matthias
[1] Julian, thanks again for catching this silly typo in
app-emulation/libvirt :-D
[2] notwithstanding whether the comments are correct/justified on a
purely technical level.
[3] again app-emulation/libvirt
