node-report
node-report copied to clipboard
nodejs
Add nodestyle and lint code
This looks good and I think we should merge it in but I have one major issue and one minor one. The main issue is this only checks the .js code, on it's own it doesn't close issue #44 as most of the code for this project is the C/C++ which actually generates the report. It's still the right thing to do but it doesn't check the C/C++ code (what I wrote in the description of #44 might not have been clear enough).
Secondly, and more as a proper review comment, is node-style assumed to be globally installed or does it need to be added as a dependency somewhere?
@geek I see that you're a maintainer for node-style, do you happen to know how different its rules are from nodejs/node's eslint config? Also what is the benefit to using it over eslint with the defaults (eslint:recommended).
Incidentally we're currently looking at linting in https://github.com/nodejs/citgm/pull/334, so I'm interested in the pros and cons. cc/ @gdams
@gibfahn the rules for node-style align with those used for node core. The benefit is consistency and alignment across projects.
I'm struggling to understand why if we want to be consistent with node core why we wouldn't want to use nodejs/node's eslint config
I think #446 is a better approach. The tool being added here may be a nice thing for projects that are not part of github.com/nodejs/*, but I don't think we need such a complex dependency when we only need a .eslintrc file.
If nodejs wanted to factor its eslint rules out, eslint already support this easily, https://www.npmjs.com/package/eslint-config-strongloop is an example of what we did for strongloop, it would be easy to do for node's tools, too - though node itself heavily favours "vendoring" its dependencies directly so that the repo is self-contained.
Just tried building this PR on Node.js 4.7.2 and got this:
-bash-4.2$ npm test
> [email protected] pretest /home/users/riclau/sandbox/github/nodereport
> npm run lint
> [email protected] lint /home/users/riclau/sandbox/github/nodereport
> node-style
["setTimeout","setInterval"].includes is not a function
Array.includes isn't supported in V8 4.5. Tracked it down to node_modules/node-style/node_modules/eslint-plugin-nodejs/lib/timer-arguments.js.
@richardlau eslint version? We can raise as a bug: https://github.com/eslint/eslint/blob/master/package.json#L120