GitHub user 1ambda opened a pull request:
https://github.com/apache/zeppelin/pull/1887
[ZEPPELIN-1940] most of eslint rules are NOT applied at all
### What is this PR for?
**Most fixes are about applying lint rules. It's automatically fixed using
`eslint --fix` command.**
**So please review `Gruntfile.js`, `.eslint`, `package.json` in
`zeppelin-web` directory**
As a result of the PR,
- Restored javascript lint system
- Fixed 1500+ lint errors
- Found some buggy code (e.g `Unexpected 'this' no-invalid-this` See the
screenshot section)
We are using google style guide but it is not used at all because of
invalid `.eslint` configuration. Thus I made some changes to fix it.
- Moved eslint from grunt to webpack (faster)
- Changed invalid config `"preset": "google"` to `"extends": ["google"]`
- Fixed 1500+ lint errors automatically using `eslint --fix` command
- Left some lint errors i cannot fix immediately as warnings or disabled
```
"guard-for-in": [1], // warning
"no-invalid-this": [1], // warning
"prefer-rest-params": [1], // warning
"require-jsdoc": [0], // disabled
"valid-jsdoc": [0] // disabled
```
- Also, Modified 20+ lint errors by hand
```
@ ./src/index.js 29:0-65
ERROR in ./src/app/visualization/builtins/visualization-areachart.js
Module build failed: Duplicate declaration "self"
73 | this.chart.style(this.config.style || 'stack');
74 |
> 75 | let self = this;
| ^
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/visualization.js
134:14 error Unexpected var, use let or const instead no-var
138:14 error Unexpected var, use let or const instead no-var
...
```
- Additionally, introduced `eslint:recommended` ruleset for several rules
which google style is not opinionated so that we can keep clear, unified rules.
### What type of PR is it?
[Bug Fix | Improvement]
### Todos
* [x] - Moved eslint (build) task from grunt to npm (package.json)
* [x] - Brought eslint (dev) task from grunt to webpack
* [x] - Fixed `.eslint` to applied google ruleset
* [x] - Modifed some lint errors while leaving others as warning which i
cannot fix right now (e.g docs)
* [x] - Introduced eslint recommended ruleset
### What is the Jira issue?
[ZEPPELIN-1940](https://issues.apache.org/jira/browse/ZEPPELIN-1940)
### How should this be tested?
- execute `npm run dev` and `npm run build` to check whether lint works well
### Screenshots (if appropriate)
### Image: found some invalid code using restored lint
### Image: found some errors using restored lint
### Image: Some code fixed by hand
```
â zeppelin-web git:(remove-grunt-eslint) npm run lint:js -- --quiet
> [email protected] lint:js
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web
> eslint src test Gruntfile.js "--quiet"
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/handsontable/handsonHelper.js
159:58 error Use the rest parameters instead of 'arguments'
prefer-rest-params
163:55 error Use the rest parameters instead of 'arguments'
prefer-rest-params
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/notebook/notebook.controller.js
635:20 error Unexpected var, use let or const instead no-var
648:20 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js
929:28 error Use the rest parameters instead of 'arguments'
prefer-rest-params
930:66 error Use the rest parameters instead of 'arguments'
prefer-rest-params
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/notebook/paragraph/result/result.controller.js
386:9 error Unexpected var, use let or const instead no-var
433:9 error Unexpected var, use let or const instead no-var
453:9 error Unexpected var, use let or const instead no-var
867:28 error Use the rest parameters instead of 'arguments'
prefer-rest-params
868:66 error Use the rest parameters instead of 'arguments'
prefer-rest-params
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/tabledata/transformation.js
52:7 error Unexpected var, use let or const instead no-var
54:14 error Unexpected var, use let or const instead no-var
58:14 error Unexpected var, use let or const instead no-var
77:7 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/builtins/visualization-areachart.js
61:5 error Unexpected var, use let or const instead no-var
73:5 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/builtins/visualization-barchart.js
59:5 error Unexpected var, use let or const instead no-var
67:5 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/builtins/visualization-nvd3chart.js
225:12 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/builtins/visualization-scatterchart.js
134:10 error Unexpected var, use let or const instead no-var
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/visualization.js
134:14 error Unexpected var, use let or const instead no-var
138:14 error Unexpected var, use let or const instead no-var
â 23 problems (23 errors, 0 warnings)
ERROR in ./src/app/visualization/builtins/visualization-barchart.js
Module build failed: Duplicate declaration "self"
65 | this.chart.stacked(this.config.stacked);
66 |
> 67 | let self = this;
| ^
68 | this.chart.dispatch.on('stateChange', function(s) {
69 | self.config.stacked = s.stacked;
70 |
@ ./src/index.js 29:0-65
ERROR in ./src/app/visualization/builtins/visualization-areachart.js
Module build failed: Duplicate declaration "self"
73 | this.chart.style(this.config.style || 'stack');
74 |
> 75 | let self = this;
| ^
76 | this.chart.dispatch.on('stateChange', function(s) {
77 | self.config.style = s.style;
78 |
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/src/app/visualization/builtins/visualization-nvd3chart.js
52:25 error Empty block statement no-empty
â 1 problem (1 error, 0 warnings)
```
### Questions:
* Does the licenses files need update? - NO
* Is there breaking changes for older versions? - NO
* Does this needs documentation? - NO
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/1ambda/zeppelin
ZEPPELIN-1940/apply-google-eslint-rule
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/1887.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1887
----
commit 9680f968eab4a428924992d13cea8d7037121d7e
Author: 1ambda <[email protected]>
Date: 2017-01-11T10:30:40Z
fix: Add lint:js
commit f4c37185c3d65e4185e4b196fa19ed77dc1f650f
Author: 1ambda <[email protected]>
Date: 2017-01-11T10:41:37Z
fix: Add eslint-loader
commit 3230d5252f70939b3079ebf85496e2676b69505f
Author: 1ambda <[email protected]>
Date: 2017-01-11T10:47:28Z
fix: Lint errors
> [email protected] lint:js
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web
> eslint src test Gruntfile.js
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/karma.conf.js
9:3 error 'use strict' is unnecessary inside of modules strict
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/controllers/main.js
7:14 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/controllers/nav.js
7:14 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/controllers/notebook.js
25:14 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/controllers/notename.js
8:14 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/controllers/paragraph.js
18:14 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/directives/ngenter.js
9:14 error 'inject' is not defined no-undef
13:26 error 'inject' is not defined no-undef
/Users/lambda/github/apache-zeppelin/zeppelin-master/zeppelin-web/test/spec/factory/noteList.js
8:5 error 'inject' is not defined no-undef
â 9 problems (9 errors, 0 warnings)
commit 92ce71fd767ba095939d2d593135e6dbf7e86730
Author: 1ambda <[email protected]>
Date: 2017-01-11T11:13:49Z
fix: lint errors in src, test
commit 840cd456125fcfe23e110dc8ad0b82d8e0e6900e
Author: 1ambda <[email protected]>
Date: 2017-01-11T11:22:04Z
fix: no-var, duplicated-self rules
commit e4aa14e27a9e2eb874bb0366a9be1e3ea693a285
Author: 1ambda <[email protected]>
Date: 2017-01-11T11:34:51Z
fix: Add eslint:recommended config
commit be0ec5f476d97c97e771a09bdde74b6b7a4b04c8
Author: 1ambda <[email protected]>
Date: 2017-01-11T19:59:02Z
fix: Remove lint js fix command
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
