Open Stack

On 09/07/14 17:52, Clint Byrum wrote:
> Hello!
> 
> I've been looking at the statistics, and doing a bit of review of the
> reviewers, and I think we have an opportunity to expand the core reviewer
> team in TripleO. We absolutely need the help, and I think these two
> individuals are well positioned to do that.
> 
> I would like to draw your attention to this page:
> 
> http://russellbryant.net/openstack-stats/tripleo-reviewers-90.txt
> 
> Specifically these two lines:
> 
> +-------------------+---------------------------------------+----------------+
> |      Reviewer     | Reviews   -2  -1  +1  +2  +A    +/- % | Disagreements* |
> +-------------------+---------------------------------------+----------------+
> |  jonpaul-sullivan |     188    0  43 145   0   0    77.1% |   28 ( 14.9%)  |
> |       lxsli       |     186    0  23 163   0   0    87.6% |   27 ( 14.5%)  |
> 
> Note that they are right at the level we expect, 3 per work day. And
> I've looked through their reviews and code contributions: it is clear
> that they understand what we're trying to do in TripleO, and how it all
> works. I am a little dismayed at the slightly high disagreement rate,
> but looking through the disagreements, most of them were jp and lxsli
> being more demanding of submitters, so I am less dismayed.
> 
> So, I propose that we add jonpaul-sullivan and lxsli to the TripleO core
> reviewer team.

+1 for both. However, some of the reviews show what I think is a
worrying trend in TripleO core. Specifically, nitpicking and tendency to
bikeshed.

I am absolutely in favour of keeping a clear and unified coding style --
which may require seemingly pointless comments around whitespace, using
more widespread coding idioms or requiring an explanation to a
non-obvious bit of code. Nothing against people pointing that out.

On the other hand, there is a fine line between being demanding of
submitters and slowing people down. I think asking to change the tone of
a sentence ("will" vs. "should"), requiring to replace "semver" (an
abbreviation used in the specification itself) with the full wording, or
to splitting a sentence into two, add little overall benefit and are not
something we ought to bother with.

I've seen this in the code too, but it seems much more prevalent in the
specs and docs. Every comment like this puts unnecessary burden on the
submitter, the reviewers and the CI and can delay good changes from
being merged by days or even weeks.

I know I've been guilty of this too. Can we all agree (the new as well
as the old cores) to read the bikeshedding essay again and keep it in
mind when doing reviews?

http://bikeshed.com/



> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
>