Open Stack

On Tue, May 21, 2019 at 04:37:10PM +0000, Jeremy Stanley wrote:
> On 2019-05-21 09:31:19 -0400 (-0400), Brian Rosmaita wrote:

[...]

> > 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.

/me nods in agreement.

One way I look at it (as someone who also participates in projects that
use mailing list-based patch workflows) is: 

    Would you feel comfortable sending a flurry of 'naked +1' (or "ACK")
    e-mails to patches or patch series on a publicly archived mailing
    list?  

Occasionally, yes, depending on the shared understanding between regular
contributors, and the nuanced context Jeremey notes above.  But most
reviewers (new, or otherwise) would feel awkward to do that, and instead
leave an observation (and more often: a "thoughtful assent").

The nature of Gerrit is such that ("each change is an island") it
encourages one to just push a simple button to give assent or dissent,
without additional rationale.

> > 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.

Very well articulated.


-- 
/kashyap