You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by 1ambda <gi...@git.apache.org> on 2017/01/11 21:22:05 UTC
[GitHub] zeppelin pull request #1887: [ZEPPELIN-1940] most of eslint rules are NOT ap...
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
```
\u279c zeppelin-web git:(remove-grunt-eslint) npm run lint:js -- --quiet
> zeppelin-web@0.0.0 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
\u2716 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
\u2716 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 <1a...@gmail.com>
Date: 2017-01-11T10:30:40Z
fix: Add lint:js
commit f4c37185c3d65e4185e4b196fa19ed77dc1f650f
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-11T10:41:37Z
fix: Add eslint-loader
commit 3230d5252f70939b3079ebf85496e2676b69505f
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-11T10:47:28Z
fix: Lint errors
> zeppelin-web@0.0.0 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
\u2716 9 problems (9 errors, 0 warnings)
commit 92ce71fd767ba095939d2d593135e6dbf7e86730
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-11T11:13:49Z
fix: lint errors in src, test
commit 840cd456125fcfe23e110dc8ad0b82d8e0e6900e
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-11T11:22:04Z
fix: no-var, duplicated-self rules
commit e4aa14e27a9e2eb874bb0366a9be1e3ea693a285
Author: 1ambda <1a...@gmail.com>
Date: 2017-01-11T11:34:51Z
fix: Add eslint:recommended config
commit be0ec5f476d97c97e771a09bdde74b6b7a4b04c8
Author: 1ambda <1a...@gmail.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1887: [ZEPPELIN-1940] most of eslint rules are NOT applied a...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/1887
@AhyoungRyu Thanks for caring this!
- It would be easier to start from scratch i think. \U0001f62d
- I will work on after #1982 is handed!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1887: [ZEPPELIN-1940] most of eslint rules are NOT applied a...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/1887
@1ambda Could you resolve the conflicts? Then I will review this one.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #1887: [ZEPPELIN-1940] most of eslint rules are NOT ap...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda closed the pull request at:
https://github.com/apache/zeppelin/pull/1887
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1887: [ZEPPELIN-1940] most of eslint rules are NOT applied a...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/1887
I will open another PR for this. Too many conflicts \U0001f61e
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #1887: [ZEPPELIN-1940] most of eslint rules are NOT applied a...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/1887
@1ambda Sure. No worries!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---