<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 24, 2015 at 3:14 AM, Julien Danjou <span dir="ltr"><<a href="mailto:julien@danjou.info" target="_blank">julien@danjou.info</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi there,<br>
<br>
This is now happening weekly to me now, probably because I write too<br>
many patches touching almost all OpenStack projects once a cycle, and<br>
I'm really tired of that behavior, so PLEASE:<br>
<br>
  *Stop sending Code-Review-1 when asking a question in a patch*<br>
<br>
_Sometimes_ there are good reasons to set -1 even when asking a<br>
question. For example, when the question is a hint sent to the patch<br>
author so that (s)he improves is commit message, a code comment or a<br>
piece of code.<br>
<br>
But most of the time, if you ask a question because there's something<br>
YOU DO NOT KNOW OR UNDERSTAND, do not put a score to a patchset. You<br>
don't know the answer, so you have absolutely no right to evaluate a<br>
patchset with -1. Just don't set a score, it's OK, and wait for the<br>
answer before deciding if the patch is worth [-1..+2].<br>
<br></blockquote><div><br></div><div>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.<br></div><div><br></div><div>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.<br></div><div><br></div><div>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.<br></div><div><br></div><div>- Brant<br><br></div></div></div></div>