On Tue, May 21, 2019 at 3:33 PM Brian Rosmaita <rosmaita.fossdev@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.