[GitHub] zeppelin pull request #1887: [ZEPPELIN-1940] most of eslint rules are NOT ap...

2017-03-28 Thread 1ambda
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] zeppelin pull request #1887: [ZEPPELIN-1940] most of eslint rules are NOT ap...

2017-01-11 Thread 1ambda
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](https://cloud.githubusercontent.com/assets/4968473/21866851/2937c408-d88f-11e6-9385-b9fcca6ce477.png)

### Image: found some errors using restored lint


![image](https://cloud.githubusercontent.com/assets/4968473/21866822/10966dfa-d88f-11e6-803c-bfe6f12960dd.png)

### Image: Some code fixed by hand

```
➜  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 insteadno-var
  433:9   error  Unexpected var, use let or const insteadno-var
  453:9   error  Unexpected var, use let or const insteadno-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