test: add lint rule to enforce trailing commas - #45468
Conversation
Sorry, something went wrong.
Only activated on some subfolders to minimize the diff, ideally this rule would be applied gradually to the entire codebase in follow-up commits.
|
Review requested:
|
Sorry, something went wrong.
Co-authored-by: Rich Trott <rtrott@gmail.com>
Sorry, something went wrong.
BridgeAR
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
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. |
Sorry, something went wrong.
😱 I wasn't even aware that ES permits trailing commas in function calls.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Sorry, something went wrong.
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 <rtrott@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
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 <rtrott@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
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 <rtrott@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
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.