Proposal: directly push nits to PR branches

[gibfahn]

Proposal:

For nits and non-controversial changes, collaborators can just edit the PR branch directly and add a commit.

EDIT: changed should to can

Rationale:

I've heard at least one person specifically request that nits and stylistic changes be directly pushed to their branch. I've also noticed with WG meeting minutes, where you tend to get a lot of nits, it's become common for other people to directly push changes.

The argument against doing it is that some people might object, but I think Github's Allow edits from maintainers box already covers that (if people don't want edits they can uncheck it). If lots of people object we can always revisit.

Like editing people's comments (to fix messed up formatting etc.), it's difficult to tell what the exact boundary should be. Rule of thumb would be "only do it for things that you think no-one could object to". If someone disagrees with the change they can just comment requesting the change be removed, or just directly remove it.

Technical details:

At least for single file nits this is easy, you just use the pencil button in the PR:

cc/ @nodejs/collaborators (or anyone else)

Starting the commit message with squash! would hopefully be enough to clarify that the commit should be squashed as part of landing. The Contributing and Collaborator guides would need a paragraph each with a brief overview.

It's probably good manners to rerun CI if you add a change (in case you break things).

Alternate suggestion:

We could just make the changes and post them as a diff/patch in a comment, then contributors could apply them if they wanted to (and if they don't know how it won't make their lives harder, they can just manually do the changes the patch suggests). This also removes the issue of Collaborators requesting vague changes like "can you add some more detail here", it'd become "how about this", and contributors could take it or leave it.

Refs: https://github.com/nodejs/moderation/issues/107#issuecomment-320032035

[refack]

Of the top of my head I'm +1, but there are things to consider:

1. First timers or other novices might find this aggressive
2. "Diminishing returns", if Collaborators start fixing all code nits it steals focus and brian cycles. If T(penning the nit) ~= T(fixing the code) than that's less of an issue
3. It should be well documented how to force push such commits out (also that such commits are just to help things along)
4. Preferably some criteria for what a reasonably small change, otherwise we might start bikeshedding in code instead of in words.

[Qard]

I would suggest only allowing nits to be fixed by other collaborators when only nits remain to make the PR merge ready. It would cause friction if nits we're edited while the PR author made other changes, making them potentially need to deal with merge conflicts or accidentally force push away the nit fixes.

[gireeshpunathil]

I guess T(penning the nit) << T(arbitration and ratification with the contributor)
Also I guess T(penning the nit by a collaborator) → 0

[benjamingr]

I'm for doing this only if the contributor explicitly allowed it (not sure if the checkbox is explicit enough). Asking is fine though.

I think a large part of onboarding is to get someone to make a contribution - we end up cleaning up the PR anyway (and adding the Reviewed-By and PR-URL most of the time ourselves) but that's transparent. I don't want to take away from the sense of accomplishment people get when they PR and I think this might.

I definitely see the need though.

[vsemozhetbyt]

Another possible option (as a brainstorming step):

Make a comment with some keyword phrase ("memo for the lander" etc.) and a list of nits. It should be clear that this is not a request for the PR author, but they can address it if they want.

However, this can increase PR scaffolding.

[refack]

For trying to do meta-contribution to assist experienced contributors/Collaborators, there is also the option to open a PR in the user's fork against the original PR's branch.

I've done that, and also added new code to the original PR with a commit message[suggestion] new XXX for example if I had a new test case in mind, or a few sentences to add to a doc.

There's also the option to quote the suggestion verbatim in the comment, i.e. "I would do it like":

const x = v(z);
for (const a of x) {}

[gibfahn]

@refack

For trying to do meta-contribution to assist experienced contributors/Collaborators,

If it's a collaborator's PR I think it's easier, you don't have to worry about them being upset by it (they're probably fed up of fixing nits 😁 ).

there is also the option to open a PR in the user's fork against the original PR's branch.

I think that's fine in general, but that's when you want to suggest something for discussion (other option is posting the diff in a comment). But that's a different scenario from the "trying to save you the hassle of making these changes yourself" idea.

@vsemozhetbyt

Make a comment with some keyword phrase ("memo for the lander" etc.) and a list of nits. It should be clear that this is not a request for the PR author, but they can address it if they want.

Interesting idea, we'd need automation to pick it up though, especially in something that gets a lot of review, things are likely to be missed. Also we'll probably end up with multiple people requesting the same changes.

[gibfahn]

@benjamingr

I'm for doing this only if the contributor explicitly allowed it (not sure if the checkbox is explicit enough). Asking is fine though.

Yeah, we might want to make it more explicit. I think asking will slow things down too much (by the time they've agreed you'll have forgotten what you wanted to push). Maybe we could just add a sentence to the PR template (but of course many new contributors don't get Github's weird checkbox syntax).

I don't want to take away from the sense of accomplishment people get when they PR and I think this might.

I think it really depends how much of the PR you change. If the PR is a single sentence, then rewriting half that sentence is basically rejecting their PR and doing one yourself. However if they're adding a new feature, and you're correcting a typo, I don't see it being a big deal (compared to the annoyance of reviewers just asking you to fix whitespace issues, and ignoring the rest of the code).

We can definitely err on the side of caution though, just use it sparingly (and ask beforehand), and see what new contributors think.

[gibfahn]

@Qard

I would suggest only allowing nits to be fixed by other collaborators when only nits remain to make the PR merge ready. It would cause friction if nits we're edited while the PR author made other changes, making them potentially need to deal with merge conflicts or accidentally force push away the nit fixes.

I agree it would well cause issues, although I think people struggle with rebasing on master (due to not setting pull.rebase) anyway, and doing a git fetch origin && git pull should be easier than learning how to do that.

Alternate suggestion:

We could just make the changes and post them as a diff/patch in a comment, then contributors could apply them if they wanted to (and if they don't know how it won't make their lives harder, they can just manually do the changes the patch suggests).

[benjamingr]

I'm +1 on posting a patch and +1 on fixing typo'esque nits in big features. I'm also concerned about landing more substantial nits without the regular code review cycle. Can we reach consensus on that?

[addaleax]

@benjamingr Some collaborators post approvals like “LGTM with @‍person’s nits (addressed)”, I usually take that to mean that they would approve addressing them while landing

[refack]

To elaborate on @benjamingr comment: the landing process besides being human error prone, is also amenable to the landers personal opinions and judgment to make changes that are almost invisible to review. I admit I was many times tempted to do small tweaks, or rename a variable to two while landing.
Since it's impossible to enforce, IMHO we should have at least have "should and shouldn't do" comment in the collaborators guide. Like "do fix nits" & "don't editorialize".

[mhdawson]

I have found it required a bit more work/annoying at times when nits where pushed and then I had to make additional changes on the PR. Proposing a patch is good and otherwise I think getting an ok from the PR would be good as they may just want to change them themselves.

[mcollina]

I am -1 on the current wording of this proposal:

For nits and non-controversial changes, collaborators should just edit the PR branch directly and add a commit.

I propose to change this into:

For nits and non-controversial changes, collaborators might just edit the PR branch directly and add a commit.

The difference is meaningful, and it does not create one more duty for a collaborator if they do not feel to do it. However this can just unblock some PRs that might take too much to land otherwise.

Moreover, this does not encourages newcomers to learn the style of node core. Part of the review process is "teaching". So, before pushing a collaborator should just ask and wait some time for a response, and if there is silence do the manual edits.

Generally this is ok for wg meeting notes (but it's in the WG rules, not here), and for "first time contributors". I'm -1 on doing this for active PRs by collaborators (it's still ok for PRs that are stuck and the collaborator is not engaging anymore).

[gibfahn]

@mcollina changed should to can, (might sounds a bit odd to me).

Moreover, this does not encourages newcomers to learn the style of node core. Part of the review process is "teaching". So, before pushing a collaborator should just ask and wait some time for a response, and if there is silence do the manual edits.

I think showing a contributor what you mean still counts as teaching. Asking them to make changes is fine if you're specific, but for stylistic things we should show them with the linter. I think particularly for first time contributors, we should try to ease their entry into the world of PR review.

[mcollina]

@mcollina changed should to can, (might sounds a bit odd to me).

👍

I think particularly for first time contributors, we should try to ease their entry into the world of PR review.

👍 , we should try to do this for first-time contributors as much as possible.

[Trott]

Closing as stale and possibly resolved. Please feel free to re-open if you think it warrants it.