<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>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...<br>
</p>
<p>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. <br>
</p>
<p>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.</p>
<p>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.<br>
</p>
<p>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.</p>
<p>A few questions:<br>
</p>
<ol>
<li>Who knew about the "How to Review Changes the OpenStack Way" document before this thread?</li><ol>
<li>If you did, how did you find the content? Did you find it easy to understand and follow?</li><li>If you did not, would you have found this document helpful when you first started reviewing?<br>
</li></ol>
<li>PTLs - do you share this document with new contributors that are reappearing?</li><li>Is this socialised in the Upstream Institute meeting? (@diablo)</li></ol>
<p>I don't expect a flood of answers to these questions, but it's important that these are on our mind.<br>
</p>
<p>Cheers,</p>
<p>Alex<br>
<br>
IRC: asettle<br>
Twitter: @dewsday<br>
</p>
<p>[0]: <a class="moz-txt-link-freetext" href="https://docs.openstack.org/project-team-guide/review-the-openstack-way.html">
https://docs.openstack.org/project-team-guide/review-the-openstack-way.html</a> <br>
</p>
<div class="moz-cite-prefix">On 22/05/2019 11:02, Natal Ngétal wrote:<br>
</div>
<blockquote type="cite" cite="mid:CAF+jc3WyBuc86=XtNMBBL_zXKvFscMm+EtZwoc-kNeZx5uY79g@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">On Tue, May 21, 2019 at 3:33 PM Brian Rosmaita
<a class="moz-txt-link-rfc2396E" href="mailto:rosmaita.fossdev@gmail.com"><rosmaita.fossdev@gmail.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">A recent spate of +1 reviews with no comments on patches has put me into
grumpy-old-man mode.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I totally agree a -1 must be coming with a comment, it's a nonsense to
have a -1 without explanation.
</pre>
</blockquote>
</body>
</html>