Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Add nodestyle and lint code #45

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add nodestyle and lint code #45

wants to merge 1 commit into from

Conversation

geek
Copy link
Member

@geek geek commented Jan 25, 2017

Closes #44

@hhellyer
Copy link
Contributor

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?

package.json Outdated
"test": "tap test/test*.js"
},
"bugs": {
"url": "https://github.com/nodejs/nodereport/issues"
},
"devDependencies": {
"node-style": "^1.1.0",
Copy link
Member

@gibfahn gibfahn Jan 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hhellyer node-style is added as a devDependency here (i.e. it won't show up if you npm i node-report, but it will show up if you cd node-report && npm i.

@gibfahn
Copy link
Member

gibfahn commented Jan 25, 2017

@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 nodejs/citgm#334, so I'm interested in the pros and cons. cc/ @gdams

@geek
Copy link
Member Author

geek commented Jan 25, 2017

@gibfahn the rules for node-style align with those used for node core. The benefit is consistency and alignment across projects.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the assertion that these are now the same as used for node core, LGTM

@gdams
Copy link
Member

gdams commented Jan 31, 2017

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking deeper sounds like we need more discussion. What I'm thinking is that we really just want something which is the same as in node core, if we need something more then we should explain the case for that. So as George mentioned why would we not just use the nodejs/node's config.

@sam-github
Copy link
Contributor

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.

@richardlau
Copy link
Member

Just tried building this PR on Node.js 4.7.2 and got this:

-bash-4.2$ npm test

> nodereport@1.0.7 pretest /home/users/riclau/sandbox/github/nodereport
> npm run lint


> nodereport@1.0.7 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.

@sam-github
Copy link
Contributor

@richardlau eslint version? We can raise as a bug: https://github.com/eslint/eslint/blob/master/package.json#L120

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants