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

.eslintrc: enable es6's spread #5028

Closed
wants to merge 3 commits into from

Conversation

thefourtheye
Copy link
Contributor

This came up in
#5020 (comment). This
patch basically enables spread option so that eslint will be happy.

This came up in
nodejs#5020 (comment). This
patch basically enables spread option so that eslint will be happy.
@thefourtheye thefourtheye added the tools Issues and PRs related to the tools directory. label Feb 1, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2016

LGTM, but do we need this if we aren't using it anywhere yet?

@thefourtheye
Copy link
Contributor Author

Hmmm, that is true. But unless we enable, no code can use spread operator, right?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2016

That's true, but in that case, we might as well just turn everything on. I think in future versions of ESLint we'll be able to just enable an entire language version.

@thefourtheye
Copy link
Contributor Author

@cjihrig Fair enough. I included all the relevant es6 things from http://eslint.org/docs/user-guide/configuring.html#specifying-language-options. PTAL.

@thefourtheye
Copy link
Contributor Author

Oh wait! There are linter errors now.

@thefourtheye
Copy link
Contributor Author

Okay, fixed now. All because of the usage of 'use strict' in modules, when modules rule was enabled.

@silverwind
Copy link
Contributor

LGTM pending CI.

CI: https://ci.nodejs.org/job/node-test-pull-request/1484/

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2016

LGTM

1 similar comment
@targos
Copy link
Member

targos commented Feb 1, 2016

LGTM

@targos
Copy link
Member

targos commented Feb 1, 2016

Btw I just tried eslint 2 (which is still in beta) here: https://github.com/targos/node/commits/eslint-v2
The upgrade will be quite easy and we'll be able to replace this list with parserOptions: { ecmaVersion: 6 } in the config.

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

LGTM

1 similar comment
@jbergstroem
Copy link
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Feb 3, 2016
This change enables the es6 environment in eslint. Rather than selecting
es6 rules one at a time, this turns all of them on with the exception of
modules.

Ref: nodejs#5028
@Trott
Copy link
Member

Trott commented Feb 3, 2016

@cjihrig wrote:

That's true, but in that case, we might as well just turn everything on. I think in future versions of ESLint we'll be able to just enable an entire language version.

I think that day is here already. #5052

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2016

I was referring to the upcoming ecmaVersion that @targos mentioned, but just setting the env is fine too :-)

@thefourtheye
Copy link
Contributor Author

Closing in favor of #5052

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants