Node: What is our policy towards stylistic changes?

[benjamingr]

Hey,

I recently saw a PR here: https://github.com/nodejs/node/pull/35603 opened by a first time contributor containing (only) stylistic changes.

I am not sure what our policy regarding such PRs are:

• I know in the distant past we did not merge those but in the more-recent past we did and even encouraged those.
• Looking a year ago - code-and-learn tagged issues are filled with such (stylistic) changes.

---

What is our policy towards stylistic changes?

And is that policy different for first time contributors / non-collaborators in general?

[Trott]

Here's how I've thought about it:

Style-only changes in a single file that make it internally consistent are ✅.

For multiple files, things get more subjective and I'm not sure a concrete policy is going to emerge. It doesn't take much to introduce backporting problems. My tolerance goes up, though, if there is a lint rule with an auto-fixer (because that makes backporting easy and prevents backsliding). And then there's also the question of what people think of the style in particular. A style change that conforms to most people's practices (e.g., braces around conditional blocks even if they are a single line) might be permitted more churn whereas more unusual style changes (e.g. leading commas or Yoda conditions) might not be acceptable at all in a style-only PR.

[mscdex]

In general such PRs (single or multi-file changes) are -1 from me because style-only changes don't add any real value to the project (and only complicate git blaming), unlike other types of PRs. The one exception to this I could see is if someone submits a PR containing a meaningful replacement of a (large) chunk of code.

Any styles we think are important enough to enforce should go in the linter configuration. However, at that point any style issues would be solved by every PR, negating the need for individual style-only PRs.

[benjamingr]

@mscdex my understanding is that the value of these PRs is that they build competence in potential future collaborators and get them acquainted with our process.

[Trott]

@mscdex my understanding is that the value of these PRs is that they build competence in potential future collaborators and get them acquainted with our process.

@benjamingr I'm guessing you'll agree, but just to get this cleared up quickly: I don't think we should land changes that don't actually improve the code base. (I'm using "code base" broadly. Documentation counts and all that.)

I think your point is that the improvement need not be large. (I would agree with that.)

[jasnell]

Changes need not be large or substantial, but we have avoided purely formatting related stylistic changes pretty consistently -- e.g. we're not likely to accept a PR that does nothing more than change whitespace or line wrapping in a file (unless, of course, it was a linter fix).