Replying to the last thread and top posting both on purpose as I want to point out something slightly differently but I am taking this platform as a way to create further discussion...
The topic of +1 and -1 was brought up in a number of times at the most recent PTG. In the past it has been somewhat obvious that we HAVE had individuals who provide a +1 or -1 to improve their review stats. Now, whilst we *now* have the guide to reviewing as Julia pointed out [0] we originally created a community that kind of had to make up the meanings behind a +1 and -1. I am speaking generally here, some projects definitely had defined rules as to what these reviews meant, but not all did. This obviously left this very open to interpretation.
To become a core review in many projects, the barrier to entry was general "you must provide quality reviews". That's great, except what ended up happening is we had waves of both individuals providing mass +1's and -1's. Neither were quality.
When we (the docs team) were looking at who to offer core responsibilities to, we used to look specifically at the amount of +1's and -1's. At that point in time, a -1 was seen to be a review of "higher quality" because it meant somehow that the individual's critical review, automatically meant "critical thinking". All this did was create a negative review culture ("a -1 is better than a +1"). I know I used to be highly conscious of how many +1's I offered before I became core, I would often avoid providing a +1 even if the patch was fine so my stats would not be messed up. Which is pretty messed up.
We have the review document in the project team guide, but what are we doing to socialise this? This thread currently is divulging into personal opinions and experiences (myself included here) but it looks like we need to work on some actions.
A few questions:
1. Who knew about the "How to Review Changes the OpenStack Way" document before this thread? * If you did, how did you find the content? Did you find it easy to understand and follow? * If you did not, would you have found this document helpful when you first started reviewing? 2. PTLs - do you share this document with new contributors that are reappearing? 3. Is this socialised in the Upstream Institute meeting? (@diablo)
I don't expect a flood of answers to these questions, but it's important that these are on our mind.
Cheers,
Alex
IRC: asettle Twitter: @dewsday
[0]: https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
On 22/05/2019 11:02, Natal Ngétal wrote:
On Tue, May 21, 2019 at 3:33 PM Brian Rosmaita rosmaita.fossdev@gmail.commailto: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.