Open Stack

On Tue, May 21, 2019 at 3:33 PM Brian Rosmaita
<rosmaita.fossdev at gmail.com> wrote:
> A recent spate of +1 reviews with no comments on patches has put me into
> grumpy-old-man mode.
What do you mean by this? I make several code review and each time I
add +1 that will mean I have made a code review.

> A +1 with no comments is completely useless (unless you have a review on
> a previous patch set with comments that have been addressed by the
> author).  I already know you're a smart person (you figured out how to
> sign the CLA and navigate gerrit -- lots of people can't or won't do
> that), but all your non-comment +1 tells me is that you are in favor of
> the patch.  That doesn't give me any information, because I already know
> that the author is in favor of the patch, so that makes two of you out
> of about 1,168 reviewers.  That's not exactly a groundswell of support.
I don't make code review to improve my statics, however sometimes I
add a +1 and I have don't saw an error and another saw an error.
That doesn't means I have don't read the code, sometimes during a code
review we can also make a fail. This why it's good to have many
different person make the code review on a patch.

> When you post your +1, please leave a comment explaining why you
> approve, or at least what in particular you looked at in the patch that
> gave you a favorable impression.  This whole open source community thing
> is a collaborative effort, so please collaborate!  You comment does not
> have to be profound.  Even just saying that you checked that the release
> note or docs on the patch rendered correctly in HTML is very helpful.
For me a +1 without comment is ok. The +1 is implicit that mean looks
good to merge, but must be review by another person. Impose to add a
comment for each review, it's for me a nonsense and a bad idea.
Which type of comment do you will? I mean if it's only to add in the
comment, looks good to merge, that change nothing.

> The same thing goes for leaving a -1 on a patch.  Don't just drop a -1
> bomb with no explanation.  The kind of review that will put you on track
> for becoming core in a project is what johnthetubaguy calls a
> "thoughtful -1", that is, a negative review that clearly explains what
> the problem is and points the author in a good direction to fix it.
I totally agree a -1 must be coming with a comment, it's a nonsense to
have a -1 without explanation.