Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ESLint no-unused-vars rule #1497

Merged
merged 1 commit into from Aug 2, 2018

Conversation

@maclover7
Copy link
Member

commented Jul 8, 2018

Checklist
Description of change

Adds the no-unused-vars ESLint rule. Can add npm run lint to nodegyp-test-commit if people would like that.

@maclover7 maclover7 requested a review from bnoordhuis Jul 8, 2018
@maclover7

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

@maclover7

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

@cclauss cclauss referenced this pull request Jul 22, 2018
0 of 4 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Can add npm run lint to nodegyp-test-commit if people would like that.

I'd rather we add npm run lint to npm test (see #1336 (comment))

},
"scripts": {
"test": "tape test/test-*"
"lint": "node_modules/eslint/bin/eslint.js --no-eslintrc --rule no-unused-vars:error bin lib test",

This comment has been minimized.

Copy link
@gibfahn

gibfahn Jul 25, 2018

Member

Can you not just do:

-     "lint": "node_modules/eslint/bin/eslint.js --no-eslintrc --rule no-unused-vars:error bin lib test", 
+     "lint": "eslint --no-eslintrc --rule no-unused-vars:error bin lib test", 

Also is there a reason to add the lint rules here instead of just adding a .eslintrc.yaml? I assume we're going to add more rules in the future...

This comment has been minimized.

Copy link
@maclover7

maclover7 Jul 29, 2018

Author Member

Had listed just the one rule for now, since didn't want to introduce tons of style changes all at once (and different parts of the codebase follow different standards...)

This comment has been minimized.

Copy link
@gibfahn

gibfahn Jul 30, 2018

Member

Oh yeah, I didn't mean that you should add a full set of rules, just that adding a second rule will be a lot easier if it's just adding a line to a yaml file.

I think using .eslintrc.yaml is the convention in everything but nodejs/node (and that's a recent change).

@maclover7

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2018

@gibfahn Added npm run lint to npm test, but looks like CI came back with some interesting Windows errors (cc @joaocgreis): https://ci.nodejs.org/job/nodegyp-test-pull-request/68/

@joaocgreis

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

@maclover7 fixed the job error - npm.cmd was not being copied to path so npm could not be used from CMD.

Test run: https://ci.nodejs.org/job/nodegyp-test-commit/391/nodes=win2016-vs2017/console

About the new error, can you use just eslint (installed by npm to node_modules/.bin) instead of the full path?

@maclover7 maclover7 force-pushed the maclover7:jm-lint-vars branch from 240d943 to e5da8fc Jul 31, 2018
@maclover7

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

CI (https://ci.nodejs.org/job/nodegyp-test-pull-request/77/) is passing now besides for Node.js v4.x, but support for that release line is being removed soon from the master branch.

PTAL @gibfahn @joaocgreis

@joaocgreis

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Windows fix LGTM. The rest of the diff looks reasonable at a glance but I didn't test.

@gibfahn
gibfahn approved these changes Aug 1, 2018
Copy link
Member

left a comment

Seems reasonable.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

@maclover7 just to confirm, all the fixes come from eslint warnings right?

@maclover7

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@maclover7 just to confirm, all the fixes come from eslint warnings right?

@gibfahn Yep, they came from when I ran npm test

- Uses `.eslintrc.yaml` for configuration
- `npm run lint` is part of `npm test`
@maclover7 maclover7 force-pushed the maclover7:jm-lint-vars branch from fd0b898 to 71f3c34 Aug 1, 2018
@maclover7

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@maclover7 maclover7 merged commit b2e5cf0 into nodejs:master Aug 2, 2018
@maclover7

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2018

Landed in b2e5cf0

@maclover7 maclover7 deleted the maclover7:jm-lint-vars branch Aug 2, 2018
@joaocgreis joaocgreis referenced this pull request Oct 11, 2018
2 of 2 tasks complete
rvagg added a commit that referenced this pull request Apr 24, 2019
- Uses `.eslintrc.yaml` for configuration
- `npm run lint` is part of `npm test`

PR-URL: #1497
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@rvagg rvagg referenced this pull request Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.