Open Stack

On Fri, Apr 24, 2015 at 3:14 AM, Julien Danjou <julien at danjou.info> wrote:

> Hi there,
>
> This is now happening weekly to me now, probably because I write too
> many patches touching almost all OpenStack projects once a cycle, and
> I'm really tired of that behavior, so PLEASE:
>
>   *Stop sending Code-Review-1 when asking a question in a patch*
>
> _Sometimes_ there are good reasons to set -1 even when asking a
> question. For example, when the question is a hint sent to the patch
> author so that (s)he improves is commit message, a code comment or a
> piece of code.
>
> But most of the time, if you ask a question because there's something
> YOU DO NOT KNOW OR UNDERSTAND, do not put a score to a patchset. You
> don't know the answer, so you have absolutely no right to evaluate a
> patchset with -1. Just don't set a score, it's OK, and wait for the
> answer before deciding if the patch is worth [-1..+2].
>
>
If other developers can't understand your code then the code should be
changed to be clearer. There's no reason for OpenStack code to be so
complex that a developer can't understand it. Having code that developers
don't understand leads to bugs and sometimes security vulnerabilties that
affect our users.

There's also be a corollary to this -- don't +1 (and especially +2) code
that you don't understand. In the past I've ignored changes that I wasn't
going to understand, typically this has been because it's implementing an
external standard that I'm not familiar with and don't want to spend hours
reading the spec. These changes tend to sit around unreviewed for a long
time, so it would help the submitter to make the code and reasoning
clearer. So there's a flip side to this -- if reviewers are going to have
trouble understanding the change then expect it to take a long time to
merge, if it ever does.

Maybe it's worth it to have a job that removes -1s after some time, so the
reviewer will know to go back and check on it.

- Brant
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150424/cd0c16a7/attachment.html>