Open Stack

> 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