-
Notifications
You must be signed in to change notification settings - Fork 45
eslint: add eslint to nodereport #46
base: main
Are you sure you want to change the base?
Conversation
adds eslint to be run witn `npm test`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the use of eslint:recommend
, this approach looks simple and very useful.
no-constant-condition: 0 | ||
no-regex-spaces: 0 | ||
|
||
extends: eslint:recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't the same as nodejs/node: https://github.com/nodejs/node/blob/master/.eslintrc.yaml
Pretty much everyone from Node in a recent thread (that I can't find at the moment, sorry) heavily favours explicit specification, its quite painful updating between eslint versions when using their builtins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the other modules such as citgm don't currently hard code all the rules. The thread was a PR that I submitted the other day. I'm happy to define them all but is it really necessary for a module that isn't primarily javascript anyway? If we implement a CI such as travis we would very quickly pick up new rules that break things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the easy thing to just directly copy node's file? I've had horrendous times in the past when eslint bumps majors trying to update, and they used to bump majors or introduce incompatibilities pretty often. From what I saw, I'm not the only one. If its easy, I would just copy the file from nodejs, if that is more complex than it seems (perhaps it relies on some custom rules?), then I've no objection to this. Any eslint is better than none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't hold me to it, but I believe that the community uses several custom rules, i'll double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do, it's in tools/eslint-rules.
I don't think it makes a big difference whether we include the rules or not, it shouldn't be that hard to include them if it means possibly less work down the line.
Adds eslint to be run with
npm test
. We should use eslintrc to remain consistent with the nodejs community such as nodejs/node and nodejs/citgm. This also allows you to usenpm run lint
ornpm run lint --- --fix
to fix linter errors.I have also shortened the timeout to ten seconds for the tap tests
Alternative to: #45