> On Apr 24, 2015, at 06:27, Brant Knudson <b...@acm.org> wrote: > > > >> On Fri, Apr 24, 2015 at 3:14 AM, Julien Danjou <jul...@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. >
This is really an important reason why -1 with a question cannot be simply "not done". If I don't understand the code, or what will happen in a specific case, a -1 is more useful than a no score. Complex code (or really clever code) is hard to maintain. --Morgan > 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 > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
