Open Stack

100% agree here with Morgan. +1 is great, especially for projects with fewer regular reviewers where more eyes can help a lot.

Best Regards, Erik Olof Gunnar Andersson

From: Morgan Fainberg <morgan.fainberg at gmail.com>
Sent: Tuesday, May 21, 2019 10:26 AM
To: Jeremy Stanley <fungi at yuggoth.org>
Cc: openstack-discuss at lists.openstack.org
Subject: Re: [dev][all] note to non-core reviewers in all projects

<Core / Leadership Hat> I 100% agree with Jeremy here. I also want to add that a +1 without comment, even from a reviewer with no history, is an indicator that the code was read. Everyone has to start somewhere. I am 100% ok with some bare/naked +1s as long as eventually those folks give quality feedback (even as far as a -1 with a "hey this looks off" or a no-score with a question). If it is a pattern of just +1 to their friends/coworkers patches without review, I'll eventually ignore the +1s.

In short, I encourage +1 even without comment if it brings in new contributors / reviewers. I hope that if there is a subsequent -1 on a patch (or other comments) the reviewers giving the initial +1 will read the comments and understand the reasoning for the -1 (-2, -1 workflow, whatever).

I think what I am saying is: I hope we (as a community) don't chase folks off because they +1'd without comment. Everyone starts somewhere

On Tue, May 21, 2019 at 9:42 AM Jeremy Stanley <fungi at yuggoth.org<mailto:fungi at yuggoth.org>> wrote:
On 2019-05-21 09:31:19 -0400 (-0400), Brian Rosmaita wrote:
> A recent spate of +1 reviews with no comments on patches has put
> me into grumpy-old-man mode.

Welcome to the club, we probably have an extra lapel pin for you
around here somewhere. ;)

> 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 think it's far more contextually nuanced than that. When I see a
naked +1 comment on a change from a reviewer I recognize as having
provided insightful feedback in the past, I tend to give that some
weight. Ideally a +1 carries with it an implicit "this change makes
sense to me, is a good idea for the project, and I don't see any
obvious flaws in it." If I only ever see (or only mostly see) them
from someone who I don't recall commenting usefully in recent
history, I basically ignore it. The implication of the +1 for that
reviewer is still the same, it's just that I don't have a lot of
faith in their ability to judge the validity of the change if they
haven't demonstrated that ability to me.

> 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.
[,...]

In an ideal world where we all have plenty of time to review changes
and whatever else needs doing, this would be great. You know better
than most, I suspect, what it's like to be a core reviewer on a
project with a very high change-to-reviewer ratio. I don't
personally think it's a healthy thing to hold reviews from newer or
less frequent reviewers to a *higher* standard than we do for our
core reviewers on projects. The goal is to improve our software, and
to do that we need a constant injection of contributors with an
understanding of that software and our processes and workflows. We
should be giving them the courtesy and respect of assuming they have
performed diligence in the course of a review, as encouragement to
get more involved and feel a part of the project.

As project leaders, it falls upon us to make the determination as to
when feedback from a reviewer is pertinent and when it isn't, *even*
if it requires us to keep a bit of context. But more importantly, we
should be setting the example for how we'd like new folks to review
changes, not simply telling them.
--
Jeremy Stanley
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20190521/9dcb85b3/attachment-0001.html>