lib: change "comma-dangle" rule

[daynin]

• change the rule
• fix all linting issues
• close Change "comma-dangle" rule #19131

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

lib, doc

[devsnek]

1384 changed files is an unacceptable amount of churn for one pr, i'm -1 on this

[daynin]

@devsnek but how can we do this gradually? One PR for one file?

[cjihrig]

-1. This is so much churn. And no, please don't do this as multiple PRs.

[gibfahn]

@devsnek but how can we do this gradually? One PR for one file?

What is the benefit to enforcing one rule here? The only benefit I am aware of with the dangling comma is that it reduces the size of diffs. Making this many changes in order to reduce diff size seems counter-productive.

it can create misunderstandings in code review process.

Could you go into more detail please? AFAIK you're free to use either style in the codebase. I prefer trailing commas, but that's not something I'd request changes for in a PR review.

[apapirovski]

I was going to suggest just adding this rule for lib but it looks like we recently went away from having separate lint configs on per directory basis. As per my comment on the original issue, I think doing this on an ongoing basis and being hopeful that eventually we get to the point where this rule can be enabled, is probably the most reasonable outcome.

[gibfahn]

I think doing this on an ongoing basis

Do you mean that people raising PRs (for some other reason) to a section of code could add commas at the same time? If so then agreed.

[BridgeAR]

@apapirovski

I was going to suggest just adding this rule for lib but it looks like we recently went away from having separate lint configs on per directory basis.

If that is the case, then it is a regression. We still have the .eslintrc.yaml files in the directories that should be there for exactly that. I did not check if they still work or not.

[apapirovski]

@BridgeAR Seems my editor (Atom) recently decided to start ignoring these hidden files... ugh. I guess I have to hunt down what changed.

Do you mean that people raising PRs (for some other reason) to a section of code could add commas at the same time? If so then agreed.

@gibfahn Yeah, exactly.

[BridgeAR]

@apapirovski it is very likely #18566.

[BridgeAR]

I am going to close this due to the mentioned concerns.

@daynin thanks a lot for your contribution anyway!

[daynin]

@BridgeAR ok. And I'll close this issue too #19131

[gibfahn]

Thanks for taking the trouble @daynin, and for the PRs to node! The effort is appreciated and the discussion is useful even if the decision is not to change things.

[BridgeAR]

@apapirovski I just checked if the .eslintrc.yaml in e.g. ./lib is still used and for me it does seem to be the case.