Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uptest,tools: refactor custom ESLint for readability #21134
Conversation
nodejs-github-bot
added
the
tools
label
Jun 5, 2018
lpinca
reviewed
Jun 5, 2018
| 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "name", "number")', | ||
| 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "name", "boolean")', | ||
| 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "name", "null")', | ||
| 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "name", "undefined")', |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Trott
Jun 6, 2018
Author
Member
Are we moving to trailing commas?
@lpinca We use both styles in the code base. I would never block or even nit anyone else's PRs about it, but yes, I try to add trailing-commas when the opportunity presents itself.
FWIW, right now on master, if we change the comma-dangle rule to this…:
'comma-dangle': ['error', {
arrays: 'always-multiline',
objects: 'only-multiline',
}],…we get 689 lines to change across 367 files. That's not something I'd want to do in a single comma-dangle PR but also something that seems achievable over enough time. (We've done it for other rules.)
In #19133, it seemed agreed that this approach (change when you can if you're already in there for other reasons) was a reasonable approach.
This comment has been minimized.
This comment has been minimized.
lpinca
Jun 6, 2018
Member
I'm not suggesting to enable the rule or blocking this for such a nuance, I was only only interested to know if any decision was made about that. In a recent PR discussion (I'll try to find it and link here) some collaborators, me included, were not very happy with trailing commas.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Trott
Jun 6, 2018
Author
Member
@lpinca I suspect you mean the voting on this comment: #20703 (comment)
You, tniessen, and cjihrig went for "no trailing comma".
A little later on, bnoordhuis, targos, and mmarchini seemed to be in favor of it (definitely bnoordhuis, and inferring the other two from the GitHub reactions on #20703 (comment)).
Dangling-comma vs. no-dangling-comma may finally be impossible for the project to resolve unlike the venerable tabs vs. spaces and semicolons vs. no-semicolons.
This comment has been minimized.
This comment has been minimized.
Trott
Jun 6, 2018
Author
Member
I was only only interested to know if any decision was made about that.
In that case: Definitely not. No decision has been made.
This comment has been minimized.
This comment has been minimized.
lpinca
approved these changes
Jun 6, 2018
trivikr
approved these changes
Jun 6, 2018
Trott
added
the
author ready
label
Jun 6, 2018
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Jun 7, 2018
This comment has been minimized.
This comment has been minimized.
|
Landed in cdd2e96 |
Trott commentedJun 5, 2018
Refactor the test and the source for the
lowercase-name-for-primitivecustom ESLint rule for readability.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes