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@gmail.com> Sent: Tuesday, May 21, 2019 10:26 AM To: Jeremy Stanley <fungi@yuggoth.org> Cc: openstack-discuss@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@yuggoth.org<mailto:fungi@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