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

Switch ClosureLinter to other linter. #1253

Closed
yosuke-furukawa opened this issue Mar 25, 2015 · 6 comments
Closed

Switch ClosureLinter to other linter. #1253

yosuke-furukawa opened this issue Mar 25, 2015 · 6 comments
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.

Comments

@yosuke-furukawa
Copy link
Member

I am watching this discussion, #1243 .
This discussion is not suitable for discuss linter tools.

I have an interest to switch closure-linter to others.

We could not write template string literal in our codes, because closure linter does not recognize new syntax like "". I guess closure linter dose not recognize arrow function =>and generator functionfunction*`.
I think this closure-linter is to be obstacle to change our code to ES6.

So I would like to switch closure-linter to other linters like eslint, jshint.
IMO, I know eslint supports ES6 syntax. http://eslint.org/blog/2014/11/es6-jsx-support/

If @iojs/collaborators agree with this issue, I will send a pull request for this.

@yosuke-furukawa
Copy link
Member Author

@chrisdickinson I guess you also have an interest to switch other linter. #1243 (comment)
This work is great!

@mscdex mscdex added discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project. labels Mar 25, 2015
@brendanashworth
Copy link
Contributor

+1. However, we have to decide whether we should be able to run the linter without building io.js; the closure linter was written in Python, while both jshint and eslint are in JavaScript.

@jbergstroem
Copy link
Member

I'd be ok with only being able to execute things like tests, benchmarks and qa tools after a build.

@chrisdickinson
Copy link
Contributor

Yep, the previous status was that we'd be okay switching to eslint / javascript-based tools, since they run after the tests (and thus, we can assume we have at least one working iojs around).

I lean towards eslint (or eslint+jscs), since it's got a pluggable rule system. I have a work in progress branch that switches things over, if anyone would like to take it over. The difficulty was striking a balance between leaning useful rules on and avoiding code churn.

@silverwind
Copy link
Contributor

eslint looks pretty flexible. One thing I'd like to avoid is wrapping code in exclusion rules like this:

/* eslint-disable new-cap */
DTRACE_HTTP_CLIENT_REQUEST(this, this.connection);
COUNTER_HTTP_CLIENT_REQUEST();
/* eslint-enable new-cap */

The new-cap rule assumes that the any capitalized variable is a constructor. Maybe the 'pluggable' system would allow us to add special rules like for these DTRACE constants?

yosuke-furukawa added a commit that referenced this issue May 9, 2015
PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
yosuke-furukawa added a commit that referenced this issue May 9, 2015
PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@yosuke-furukawa
Copy link
Member Author

Switched closure-linter to eslint! Thanks! f9dd34d 19ffb5c

Fishrock123 pushed a commit to Fishrock123/node that referenced this issue May 19, 2015
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue May 19, 2015
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

6 participants