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

[mikeal]

I think there is broad agreement that all of these issues and nits should be fixed before being merged, I don't think that is what is up for debate. What is up for debate is how we move the contributor towards resolving those issues.

The problem I believe we are trying to address is that "request changes" DOES NOT ONLY mean "fix these before merge." Any comment on the PR accomplishes this. What the "request changes" feature effectively means is "fix these for me and I will need to approve this again after you've completed the change" and it does so in a pretty forceful way that is less obvious to people who review everyday but very obvious to people who contribute occasionally.

Before this feature existed the process never blocked on an individual this way unless it was a deep technical concern that others weren't capable of reviewing. All we had were comments about adjustments and after they were dealt with any number of people might come back in and give a +1 to the review as a whole after the changes.

As a side benefit, this meant that all comments were relatively "soft." They did not present the user with the feeling they did something wrong, just that they had a little more to learn and to adjust their contribution appropriately.

The "request changes" features is very heavy handed. We don't have any control over how people feel about it, only on how and when we use it knowing that they will feel as though something critical needs to be changed.

IMO, scoping the use of the feature to "these are changes I will need to review again before merger" would be appropriate. For adjustments that most other reviewers can easily sign off on it seems appropriate to just use comments.

[mikeal]

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.

We've found it difficult to get people to actually use much of the authority we give them. In particular, people that are newer to being committers are more timid about things like this.

In this particular case, we're saying "go ahead and clear another reviewers objections." For the most part, we're talking about style nits and not deep technical objections where most reviewers will want the original requester to review again.

So, this is only something we expect to happen on less technical nits, which I would assume means we're encouraging our newer contributors and reviewers to do this clearing. Nothing about our experience on-boarding and educating contributors would lead me to believe they will exercise this discretion often enough.

In fact, there's no rule saying they can't do this today, and as we can see most reviewers are not comfortable doing it :(

[sam-github]

The request changes also has a technical benefit, we used to do line comments on the PR diff, and each comment generated a email notification, and the PR couldn't land as long as any of the line comments were still there. With the review UI, all the comments are batched up into one notification, and its more clear whether they have been addressed or not. @mikeal are you suggesting we avoid using the github Review UI because it sounds too much like a review, and people find it off putting? Or that we start using the "Comment" option, not the "Request Changes" option? I confess that I don't quite get it. I mean, we are requesting changes, why is making that clear a problem?

[jasnell]

The challenge with using just the Comments is that it does not show up as clearly as the Green check mark or Red X. There are a combination of issues here. (1) the big red X is discouraging to new contributors and (2) some reviewers tend to be ... abrupt... in how they provide their feedback. Used in combination, the experience can be rather off-putting. I have found myself often wishes GitHub had added a fourth Yellow option to indicate minor changes needed while reserving the big Red X for actual blocking issues.

[mikeal]

@sam-github I'm less worried about the email notifications we send (a small annoyance to maintainers) than I am about how well we encourage and retain new contributors.

Anything we do will have a tradeoff. Small adjustments in how we review contributions, especially first time contributions, have a large aggregate impact on how many people we will retain as longer term contributors.

While it may be technically true that you are "Requesting Changes" there are a number of ways the project can convey that changes must be made to a contribution, many of which will have better retention of first time contributors than this particular feature.

[mhdawson]

I favor using comments over requesting changes when possible. If there are cases where requesting changes is used, then the onus needs to be on the person doing that to check back frequently. If a reviewer is going object in a blocking manner, then they need to actively work with the contributor to make sure it moves forwards.

[bnoordhuis]

Come to think of it, I rather like how https://github.com/nodejs/node/pulls lets you filter on No reviews / Changes requested / Approved.

I say use Changes requested more, not less! It's a good indicator of what is in progress and what is still fresh.

[addaleax]

It's a good indicator of what is in progress and what is still fresh.

The thing is, it’s not a good indicator as long as people don’t follow up; taking a look at the first page of the “Changes requested” list, there’s somewhere between 8 and 11 out of 25 PRs where the PR is not actually blocked by waiting for the author to do something.

[bnoordhuis]

Oh, that. Just dismiss reviews if the comments have been addressed and stamp your own LGTM.

I clicked a couple on the first page - no cherry-picking, I promise - and they all seem to have changes requested for valid reasons (except one, maybe.)

[refack]

BTW: I've just read that it's been online for two months but anyway github added a special badge for first time contributors

The feature came out with a tweet saying "(GitHub) Maintainers will now see "first-time contributor" badges if it's the author's first pull request to the project. Be sure to welcome them!"