Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add lint rule to enforce trailing commas #45468

Merged
merged 3 commits into from
Nov 17, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 15, 2022

Only activated on some subfolders to minimize the diff, ideally this rule would be applied gradually to the entire codebase in follow-up commits.

There have been previous attempts at imposing trailing commas via a lint rule that failed because the diff it produces it simply to large to be reviewed before git conflicts arise, let's try a more gradual approach and split the effort in separate PRs that apply only on a subset of the codebase at a time.

Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.
@aduh95 aduh95 requested a review from Trott November 15, 2022 12:59
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/wasi

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Nov 15, 2022
test/.eslintrc.yaml Outdated Show resolved Hide resolved
@Trott Trott added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 15, 2022
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about keeping function calls as they are for now? It is still relatively new that it's allowed to add trailing commas there.

Otherwise LGTM

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 15, 2022

What about keeping function calls as they are for now? It is still relatively new that it's allowed to add trailing commas there.

It's part of ES2017, like e.g. async/await; while it is indeed relatively new, we're using much more modern syntax already (e.g. optional chaining). I think we should keep them on functions, for the same reasons we want them on array and objects.

@tniessen
Copy link
Member

What about keeping function calls as they are for now? It is still relatively new that it's allowed to add trailing commas there.

😱 I wasn't even aware that ES permits trailing commas in function calls.

I think we should keep them on functions, for the same reasons we want them on array and objects.

I've never been a fan of trailing commas anywhere but this seems particularly odd. Isn't the main justification for trailing commas to reduce diff sizes when elements are added to arrays or object literals? I don't see how that would apply to, e.g., the assert.notStrictEqual call that's being modified here. Unless, of course, it is expected that arguments are frequently added or removed.

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 15, 2022

I've never been a fan of trailing commas anywhere but this seems particularly odd. Isn't the main justification for trailing commas to reduce diff sizes when elements are added to arrays or object literals? I don't see how that would apply to, e.g., the assert.notStrictEqual call that's being modified here. Unless, of course, it is expected that arguments are frequently added or removed.

It applies to function with optional parameter and variadic functions, for which we may or may not add/remove parameters in the future. For assert.notStrictEqual, which has an optional parameter, it would reduce the diff if we have to use the message parameter – in reality, it would most likely never be useful for assert.notStrictEqual given how we're using it, a better example would be e.g. array#push, or Math.max. Anyway the main reason I'm suggesting this change is because I feel it's easier if it's always mandatory rather than mixing the styles. If you feel differently, I don't mind settle on another rule.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit bd462ad into nodejs:main Nov 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in bd462ad

@aduh95 aduh95 deleted the trailing-commas branch November 17, 2022 14:13
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: #45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: nodejs#45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: #45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: #45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: #45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
Only activated on some subfolders to minimize the diff, ideally this
rule would be applied gradually to the entire codebase in follow-up
commits.

PR-URL: #45468
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants