On Tue, May 21, 2019 at 10:34 AM Marios Andreou marios@redhat.com wrote:

whilst i agree on all you wrote, imo a -1 with no comments is worse than a +1 with no comments. If you dislike my patch enough to -1 it tell me what i need to change in order to fix and get your vote

i tend to agree with this sentiment.

i am ok to have a +1 with no comment, especially from experienced team members, but a -1 should never have no comment.

peace o/

On 21/05/2019 15:47, Mohammed Naser wrote:

On Tue, May 21, 2019 at 9:37 PM Brian Rosmaita rosmaita.fossdev@gmail.com wrote:

...

Hello everyone,

A recent spate of +1 reviews with no comments on patches has put me into grumpy-old-man mode.

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 feel a bit different about this.

Most cores probably leave a +2 with no comment to merge code, the implication that a non-core needs to add more comments to it seems a bit 'unfair'.

I think this is something where we need to put our judgement in. It may seem that we get a bunch of +1s, but if there is also other -1s that have value and comments, then I think it's okay that we get +1s.

tl;dr: I don't think we should say "if you're going to give a +1, leave a comment why you agree", IMHO.

If you have no history reviewing code in the project, yes, you do need to leave a comment. I see patches that get pushed up, and then a flurry of +1's with no coments arrive from people that don't have a history of good reviews, I will discount them.

I am assuming that before someone +1s they do some sort of check, be that reading the diff line by line and agreeing with the code + direction of the patch, or running devstack and seeing if it ran, and provided the fix or feature - if you have just say what you did.

...

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.

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.

That's all I have to say. I now return to my normal sunny disposition.

cheers, brian

I've likely just not had enough coffee this morning to grok this thread, but I'd like to point out that we have a guide for how to review[0]. I've found this document very useful to help provide people with context and re-calibrate their review behaviors such that they are not detrimental to the community and collaboration.

/me goes back to coffeee

[0]: https://docs.openstack.org/project-team-guide/review-the-openstack-way.html.

On Tue, May 21, 2019 at 8:09 AM Slawomir Kaplonski skaplons@redhat.com wrote:

Hi,

...

On 21 May 2019, at 16:47, Mohammed Naser mnaser@vexxhost.com wrote:

On Tue, May 21, 2019 at 9:37 PM Brian Rosmaita rosmaita.fossdev@gmail.com wrote:

...

Hello everyone,

A recent spate of +1 reviews with no comments on patches has put me into grumpy-old-man mode.

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 feel a bit different about this.

Most cores probably leave a +2 with no comment to merge code, the implication that a non-core needs to add more comments to it seems a bit 'unfair'.

I think this is something where we need to put our judgement in. It may seem that we get a bunch of +1s, but if there is also other -1s that have value and comments, then I think it's okay that we get +1s.

tl;dr: I don't think we should say "if you're going to give a +1, leave a comment why you agree", IMHO.

I agree with that. +1 without comment still can have some value. But we should say “if you’re going to gibe a -1, please leave a comment”. That’s really important IMO.

...

...

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.

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.

That's all I have to say. I now return to my normal sunny disposition.

cheers, brian

-- Mohammed Naser — vexxhost

---

D. 514-316-8872 D. 800-910-1726 ext. 200 E. mnaser@vexxhost.com W. http://vexxhost.com

— Slawek Kaplonski Senior software engineer Red Hat

IMO, if you put +1 to a patch you should put some comments like, verified code as per standards, tested the functionality by applying patch in local environment, works but refactoring is possible.

The reason behind adding such comments is that it will help to built trust between core and non-core reviewers. This will also help the non-cores to speedup their way to become cores.

Thanks,

Abhishek

On Tue, 21 May, 2019, 20:58 Jay Bryant, jungleboyj@gmail.com wrote:

...

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.

I do not leave reviews without some sort of comment. When I was mentored into doing reviews the expectation was that you at least leave some sort of comment with any review. Also, as Graham noted, especially for people who are newer to the project this helps give information on their review. This is another one of those 'tribal knowledge' items so I am not going to get too passionate about +1's with or without comments.

...

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.

This obviously is a requirement and it is just rude to -1 with no additional direction.

...

That's all I have to say. I now return to my normal sunny disposition.

cheers, brian

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

On 21.05.2019 18:37, Jeremy Stanley wrote:

this change makes sense to me, is a good idea for the project, and I don't see any obvious flaws in it.

Would be nice to have this as a default message proposed by gerrit for +1 action. So there never be emptiness and everyone gets happy, by default!

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.

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.