Open Stack

+1 about Dan's comments.


1. We should not discourage all cases for -1 for questions. Because it
often leads more discussion about the code issue, it is helpful for such
case.

Of course, it is diffcult to find a balance point, what can -1, what can 0.
I don't think 0 in gerrit works well, because sometimes authors not care
about that.



2. Also, for typos in comments and message, -1 makes sense too.
Because 0 in gerrit means no need to improve the message and everything is
good. It can lead many bad cases for cores, and non-cores. It means it
doesn't matter if spell right or wrong.






Best Wishes,

--------------------------------------------------------------------------------
Follow your heart. You are miracle!



From:	Dan Smith <dms at danplanet.com>
To:	"OpenStack Development Mailing List (not for usage questions)"
            <openstack-dev at lists.openstack.org>
Date:	04/24/2015 10:48 PM
Subject:	Re: [openstack-dev] Please stop reviewing code while asking
            questions



> 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-request at lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150425/448a9881/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150425/448a9881/attachment.gif>