Excerpts from Doug Hellmann's message of 2015-04-24 07:23:35 -0700: > Excerpts from Julien Danjou's message of 2015-04-24 10:14:38 +0200: > > 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]. > > > > Thank you for listening, and happy hacking! > > > > In defense of those of us asking questions, I'll just point out > that as a core reviewer I need to be sure I understand the intent > and wide-ranging ramifications of patches as I review them. Especially > in the Oslo code, what appears to be a small local change can have > unintended consequences when the library gets out into the applications. > > I will often ask questions like, "what is going to happen in X > situation if we change this default" or "how does this change in > behavior affect the case where Y happens, which isn't well tested > in our unit tests." If those details aren't made clear by the commit > message and comments in the code, I consider that a good reason to > include a -1 with a request for the author to provide more detail. > Often these are cases I'm not intimately familiar with, so I ask a > question rather than saying outright that I think something is > broken because I expect to learn from the answer but I still have > doubts that I want to indicate with the -1. >
Doug I think those questions fall into the bucket of "leading, relevant questions." These are real -1 reviews. A change that doesn't have supporting evidence in the commit message, comments, and/or documentation, is often a ticking time-bomb that goes off when code is released into the wild, or sometimes much later when the what-if comes true. What I think Julien is frustrated by is not "what happens if ..." but "I don't understand how this can work" or "can we really do X in library Y?" type questions, which I've definitely seen too and have had reviews held up for months because the -1 drops it off many reviewers' radars. > Most of the time the author has thought about the issues and worked > out a reason they are not a problem, but they haven't explained > that anywhere. On the other hand, it is frequently the case that > someone *hasn't* understood why a change might be bad and the > question ends up leading to more research and discussion. > Yes, I totally agree. Commit message is a good place to explain "why" the change is a) needed, and b) safe. Comments and docstrings are also a good place to explain (b), such as '''Mutates state in X No serialization is done in this method, ensure all callers lock X first''' But again, asking "how does this new method ensure X mutations are atomic?" is an awesome leading question, and nobody is frustrated about those. So, here's what I think is a decent rule, which we might want to put in our reviewer checklist. I've submitted a review for wording that I think might be appropriate. https://review.openstack.org/177364 __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
