How we use “request changes”

[addaleax]

Hi everyone, I’d like to mention something I’ve been noticing somewhat recently: We’re a bit inconsistent about our usage of the “request changes” review button, in a way that I think does matter. It seems to me like some of us use the button very literally, i.e. to generally request changes to a PR, but I am actually trying to avoid that unless it’s something that I think should block the PR in its current state; because:

1. It often requires the reviewer to stay on top of the PR – as somebody who regularly goes through PRs that have been open for a bit and tries to figure out whether it’s okay to land them, it’s frustrating to see PRs end up in an ambiguous state where somebody requested changes, which may or may not have been addressed (the reviewer can usually tell much better than me whether that’s the case!)
2. For many people, opening a pull request against large open source projects is still a pretty scary thing, and having the first reaction be “this PR is not okay” can be really disheartening. I realize sometimes it’s the right thing to block a PR over issues with it, but be mindful of how you do that.
3. Often, it’s unnecessary – I prefer to keep “request changes” for only the set of cases in which a PR contains real bugs, or does in some as a whole way worsen the code base rather than to improve it, rather than for e.g. suggesting slightly different wordings. (As our onboarding doc says, our secondary goal for “is for the person submitting code to succeed”, immediately after improving the codebase.)

If you have thoughts on this, I’d really be curious to hear them, otherwise I’ll close this issue in a few days. (Also: I am not perfect in following these suggestions myself. I’m trying but I’d ask that you point me to instances where you have advice for handling things better.)

fyi @nodejs/collaborators

[mikeal]

This is a great writeup!

We haven't taken the time to evaluate what impact our use of many new GitHub features have been. If we decide to create a more formal policy on how to use this particular feature, which document would we put it in? Also, I assume we would add it to our on-boarding process for new contributors?

[benjamingr]

I am actually trying to avoid that unless it’s something that I think should block the PR in its current state;

I think that's the problem, I have always seen that button used when the changes requested block landing. Myself and other collaborators typically Approve and leave a "nit: ..." or "this pr can go on without this but ...".

The thing is - newcomers have a lot more blocking issues, I think it would be beneficial to have a document of common etiquette with first time collaborators, namely:

• When requesting changes, be specific and if possible point out what code should change (rather than what general behavior).
• Try to be polite and welcoming to first time collaborators, it's not a bad idea to apply this rule for general communication. Be helpful about what stages come next and when to expect them.
• When turning suggestions down, do not be dismissive of what the person is trying to say. Try to point them to research, prior art or common practices.
• Either explaining stylistic changes (like the commit message structure) or amending the commit before landing are fine - but make sure it does not drag on to a frustrating point.

I think the project has been doing a good job on these already.

[mikeal]

One other thing, it's really troublesome when several reviews have single blocking requests rather than a single reviewer with many requests. It means that after the contributor deals with the adjustments they have to wait for several people to come back and respond and, depending on the reviewers, this can take quite some time.

[jasnell]

Let me make an attempt to codify this a bit:

When reviewing a PR:

• Use the Comment review option to leave feedback on non-blocking changes that may need to be made.
• Use the Request Changes review option only to signal blocking issues.
• Use the Approve only when you feel the PR is ready to land.

In addition to this, I think we need to make it clear that a collaborator is permitted to clear a Request Changes posted by another collaborator if it's clear that the requested changes have been made.

[refack]

For many people, opening a pull request against large open source projects is still a pretty scary thing, and having the first reaction be “this PR is not okay” can be really disheartening. I realize sometimes it’s the right thing to block a PR over issues with it, but be mindful of how you do that.

👍 Since we already require approval consensus, starting with "comments" IMHO is perceived as more welcoming. Escalating to "approve" and "request changes" only when and if the PR gets momentum (I've already seen @addaleax engage in this manner, and I try to mimic)

In addition to this, I think we need to make it clear that a collaborator is permitted to clear a Request Changes posted by another collaborator if it's clear that the requested changes have been made.

👍 There are several good PRs out there that seem to be blocked in this manner...

[gibfahn]

In addition to this, I think we need to make it clear that a collaborator is permitted to clear a Request Changes posted by another collaborator if it's clear that the requested changes have been made.

I think this is key. As long as "request changes" means "once my comments have been answered I'm okay with this", then another collaborator can clear the review once the comments have been answered, which should resolve the issue of people not updating their reviews.

For many people, opening a pull request against large open source projects is still a pretty scary thing, and having the first reaction be “this PR is not okay” can be really disheartening.

Agreed. I found this post helpful: http://brson.github.io/2017/04/05/minimally-nice-maintainer

[mcollina]

There is a lot of general misunderstading on how the full review process work. A lot of people do not know how OSS work, and they feel bad about "request changes" and they fail to follow-up. I think a blog post on "do no give up if your contribution needs a change" is needed. OSS is a process to be better people and better developers.

[gibfahn]

We try to do that in CONTRIBUTING.md, but we could definitely do this elsewhere as well.

You will probably get feedback or requests for changes to your Pull Request. This is a big part of the submission process so don't be discouraged!

[sam-github]

Maybe the PR template should include a link to a post such as @mcollina describes, one that maybe walks through what's going to happen during PR review, acknowleges that is slow, and explains why. Or at least a more clear statement that the checkboxes are not decorative, they are a list of requirements.

[addaleax]

I kind of feel like people here are missing the point of what I’m trying to say. We can write policies and blogposts all we want, but that doesn’t change that it matters how we treat people.

@jasnell Sure, do you want to open a PR with that to the onboarding docs? If not, I can do that as well.

[mcollina]

@addaleax

I kind of feel like people here are missing the point of what I’m trying to say. We can write policies and blogposts all we want, but that doesn’t change that it matters how we treat people.

Agreed. I do not think we should merge code that is not ready to be merged.
The way you are proposing to manage comments is that they are not-blocking concerns, so I should land a PR if there were comments from other contributors and enough LGTMs. As a reviewer, I consider most of my comments (even little nits) as important, and they are blocking for me (they might just be a question). When sending PR, I have to address all comments before it can be landed.
This does not change the fact that we use or not use the "request change" button, that is just cosmetics for me.
I know seeing "red" frightens people, so I'm open to suggestion in how not to. (We can even decide to try to avoid it).

In addition to this, I think we need to make it clear that a collaborator is permitted to clear a Request Changes posted by another collaborator if it's clear that the requested changes have been made.

This is key for me as well.

[addaleax]

As a reviewer, I consider most of my comments (even little nits) as important, and they are blocking for me (they might just be a question).

I’m not sure we are agreeing here – isn’t the definition of a nit, as we use the term, that something can be addressed while merging the PR/later?

I think a PR is ready to be merged once it is an overall improvement to the codebase, but it sounds a bit like you don’t agree, so I’d be curious to hear what your criteria are.

In addition to this, I think we need to make it clear that a collaborator is permitted to clear a Request Changes posted by another collaborator if it's clear that the requested changes have been made.

This is key for me as well.

Ack, this is what I’ve been doing anyway. But as I’ve mentioned, usually the best person to tell whether something has been addressed or not is the original reviewer.

[mcollina]

I’m not sure we are agreeing here – isn’t the definition of a nit, as we use the term, that something can be addressed while merging the PR/later?

Yeah. But it needs to be addressed.

I think a PR is ready to be merged once it is an overall improvement to the codebase, but it sounds a bit like you don’t agree, so I’d be curious to hear what your criteria are.

Generically I land things (not just here), that are 100% an improvement to the codebase. If I am just a doubt about one part (even a nit), I'm not merging it. I had too many bad experiences of code that I merged because it was an improvement, and in the medium to long term it showed up being very hard to maintain.

Fixing the nit while merging is ok. This is something that I do not personally do, because I prefer to have a full CI run for every of the changes, but I do not want to force this on anybody.

[bnoordhuis]

I’m not sure we are agreeing here – isn’t the definition of a nit, as we use the term, that something can be addressed while merging the PR/later?

I use it for small issues that aren't structural flaws but should still be addressed before merging.

Apropos "Request changes" vs. "Comment", the former is a clear signal changes are needed, the latter I view more as neutral, informal, take-it-or-leave-it comments.

[sam-github]

I think fixing the nits (aka "quality problems") before landing is important, because without doing it we get a code base full of nits. It makes things a bit slow and discouraging, that's true, for long-time collaborators as well as new ones, but we don't have a nit cleanup squad. If there are collaborators who want to volunteer to fix the nits in someone's PR, that's great they have the time to do that, but why not do it before it lands by pushing fixups onto the PR, instead of hoping it gets done after the PR has already merged?

I occaisonally offer to do that specifically for commit messages, saying something like "this is the standard, I'll fix it up for you when I land it, but if you want to keep contributing to this project, as I hope you do, its helpful if you learn how to fix it yourself".