tools: enforce `throw new Error()` with lint rule#3714

Trott · 2015-11-08T17:58:26Z

@evanlucas left a comment on another PR pointing out that the convention in the code base is throw new Error() over throw Error(). This PR changes the five instances of throw Error() in the code base and enforces it with a linting rule.

cjihrig · 2015-11-08T18:29:58Z

tools/eslint-rules/new-with-error.js

These can be combined into a single if.

Done! Thanks!

evanlucas · 2015-11-08T20:48:23Z

LGTM

cjihrig · 2015-11-08T20:51:14Z

tools/eslint-rules/new-with-error.js

Small nit, but I don't think either set of internal parens are necessary.

~~/^[A-Z\w]*Error^/.test(arg.callee.name) should catch all other error types.~~
Edit: not quite. Should be: start - optional capital followed by \w* - Error - end, I think.

@cjihrig I'll make that adjustment.
@Fishrock123 You're after this: /^([A-Z]\w*)?Error$/ That assumes FooError() will never be the name of a function that returns an Error object and will only be a constructor for an Error object.

cjihrig · 2015-11-08T20:52:16Z

LGTM. Is it possible to extend this rule to the other built in error types (TypeError, RangeError, etc)?

Fishrock123 · 2015-11-08T21:22:25Z

LGTM, we should add the other error types too though. maybe including custom errors?

cjihrig · 2015-11-08T21:28:54Z

Maybe the types that the rule applies to should be a configuration option. Then you just have to check if arg.callee.name is in an array.

Trott · 2015-11-08T21:51:49Z

There are only five Error types listed in the docs so we could just hard-code those, allow them to be passed as options, or go with the regular expression solution above (/^([A-Z]\w*)?Error$/). I don't have a problem with extending it this way but I also want to avoid getting mired in bikeshedding. I'm inclined to just slap in the regular expression and we can always switch to a specified list later if we want to. Thoughts?

EDIT: Actually any of the three solutions seem fine to me. I don't care, as long as there's consensus from everyone else. I think any of them will work just fine for our situation.

cjihrig · 2015-11-08T23:07:31Z

I would prefer the array config solution because then the rule can be more easily extended ~~to things other than errors (new Socket(), new Server(), etc)~~. I think a regex would be more limiting in that way, and, of course, less readable.

mscdex · 2015-11-08T23:34:52Z

Maybe an array of strings and/or regexps? I think some simple regexps (e.g. /^(Type|Syntax|Range|Assertion|)Error$/) would be beneficial to prevent a huge list with many similar/related names.

cjihrig · 2015-11-08T23:41:23Z

@mscdex suggestion sounds like a winner to me.

Trott · 2015-11-09T00:20:57Z

I like the strings and regexp idea in theory, but in practice:

I don't think eslint can accept regular expressions as arguments. The arguments are specified in JSON Schema. No existing eslint rules accept regular expressions in the config file. The primitive types accepted are boolean, integer, number, string. I suppose we could accept strings and translate them into regular expressions, but yuck.
If they are all strings, it's an easy check using indexOf(). If there are regular expressions and strings mixed together, then a type check has to happen on each element. Not the end of the world, but some added complexity for functionality we may not need or use.

I'm inclined to go with an array of strings, at least initially.

Trott · 2015-11-09T00:27:55Z

Pushed a new commit with the "accept an array of Error types to check" feature. PTAL

cjihrig · 2015-11-09T00:35:12Z

I don't think eslint can accept regular expressions as arguments.

You're right. ESLint does the string to regex conversion instead.

cjihrig · 2015-11-09T16:17:19Z

tools/eslint-rules/new-with-error.js

Can you indent this and the following two lines?

cjihrig · 2015-11-09T16:19:13Z

LGTM. I'm fine with merging the rule with its current functionality.

jasnell · 2015-11-09T19:27:26Z

LGTM

Trott · 2015-11-10T15:32:01Z

@sindresorhus I proposed it but it was rejected.

I guess the next best thing would be to publish as a plugin so others can use it in their own projects. I'll get on that...

cjihrig · 2015-11-10T16:05:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/690/

Trott · 2015-11-10T16:59:07Z

@sindresorhus A first pass is now available: https://www.npmjs.com/package/eslint-plugin-new-with-error

The documentation (and probably implementation) are lacking, but feel free to open issues left and right and that should be sufficient to shame me into improving it. :-D

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`.

Add linting rule requiring `throw new Error()` over `throw Error()`.

Trott · 2015-11-11T04:27:17Z

Rebased and squashed down to two commits (one for the code changes and a second for the lint rule itself). One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/698/

jbergstroem · 2015-11-11T04:57:42Z

Never mind the ubuntu1410 slave fail. I removed it while your tests were running.

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: nodejs#3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: nodejs#3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2015-11-11T05:02:33Z

Landed in 35f2f64 and 25cd455

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott added the tools Issues and PRs related to the tools directory. label Nov 8, 2015

cjihrig reviewed Nov 8, 2015
View reviewed changes

cjihrig reviewed Nov 9, 2015
View reviewed changes

tools/eslint-rules/new-with-error.js Outdated
Copy link
Contributor
cjihrigNov 9, 2015
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you indent this and the following two lines?

Trott force-pushed the new-error branch from 1678edb to 1774e79Compare November 9, 2015 17:12

Trott added 2 commits November 10, 2015 18:06

tools: Use throw new Error() consistently
a9b3b86
In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`.

tools: enforce throw new Error() with lint rule
97ed5ec
Add linting rule requiring `throw new Error()` over `throw Error()`.

Trott force-pushed the new-error branch from 1774e79 to 97ed5ecCompare November 11, 2015 02:08

Trott closed this Nov 11, 2015

Fishrock123 mentioned this pull request Nov 11, 2015
Propose v5.1.0 #3736
Merged

rvagg mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016
V4.2.5 proposal #4768
Merged

Trott deleted the new-error branch January 13, 2022 22:29

aliscco mentioned this pull request Dec 3, 2023
[Snyk] Security upgrade lerna from 3.22.1 to 7.0.0 aliscco/alisco-node#1113
Open

Uh oh!

tools: enforce throw new Error() with lint rule#3714

tools: enforce throw new Error() with lint rule #3714

Uh oh!

Conversation

Trott commented Nov 8, 2015

Uh oh!

cjihrigNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

TrottNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Nov 8, 2015

Uh oh!

cjihrigNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123Nov 8, 2015

Choose a reason for hiding this comment

Uh oh!

TrottNov 8, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 8, 2015

Uh oh!

Fishrock123 commented Nov 8, 2015

Uh oh!

cjihrig commented Nov 8, 2015

Uh oh!

Trott commented Nov 8, 2015

Uh oh!

cjihrig commented Nov 8, 2015

Uh oh!

mscdex commented Nov 8, 2015

Uh oh!

cjihrig commented Nov 8, 2015

Uh oh!

Trott commented Nov 9, 2015

Uh oh!

Trott commented Nov 9, 2015

Uh oh!

cjihrig commented Nov 9, 2015

Uh oh!

cjihrigNov 9, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 9, 2015

Uh oh!

jasnell commented Nov 9, 2015

Uh oh!

Trott commented Nov 10, 2015

Uh oh!

cjihrig commented Nov 10, 2015

Uh oh!

Trott commented Nov 10, 2015

Uh oh!

Trott commented Nov 11, 2015

Uh oh!

jbergstroem commented Nov 11, 2015

Uh oh!

Trott commented Nov 11, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tools: enforce `throw new Error()` with lint rule#3714

tools: enforce `throw new Error()` with lint rule #3714