> 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. > > 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.
Right, and -1 makes the comment much more visible to both other cores and the reviewer. Questions which rightly point out something which would lead to what the OP considers a legit -1 can *easily* get missed in the wash of review comments on a bug. If you leave a -1 for a question and never come back to drop it when the answer is provided, then that's bad and you should stop doing that. However, I'm really concerned about the suggestion to not -1 for questions in general because of the visibility we lose. I also worry that more non-core people will feel even less likely to -1 a patch for something they feel is just their failing to understand, when in fact it's valuable feedback that the code is obscure. As a core, I don't exclude all reviews with a -1, and doing so is pretty dangerous behavior, IMHO. I'm not sure if the concern of -1s for questions is over dropping the review out of the hitlist for cores, or if it's about hurting the feelings of the submitter. I'm not in favor of discouraging -1s for either problem. --Dan __________________________________________________________________________ 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
