Re: [gentoo-dev] [RFC/announcement] Reviewers project

[Matthias Maier]

Just a comment before this discussion gets entirely side tracked.



On Mon, Oct 12, 2015, at 11:45 CDT, Ian Delaney <della5@×××××××××.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