tools: update eslint to v1.x

[targos]

This is still a WIP.

I create the PR now because there are several issues to discuss:

• The no-reserved-keys rule has been removed. It is kind of replaced by quote-props but to enable this rule, we need to choose how we want deal with quotes on object properties. The consistent option seems to be the most conservative one if the goal is to limit changes in our code.
• The indent rule changed quite a lot and has now options to control the indentation of specific cases:
  • SwitchCase: I set it to 1, meaning that the case statements need 1 level of indent (== 2 spaces) compared to the switch. There are a few places where we don't follow this rule, like here.
  • VariableDeclarator: I set it to 2. This allows us to align multiple variable declarations with var or let like:

varx=0,y=1,z=2;

I haven't pushed the lib and test changes yet because I think there are some problems in eslint with the indent rule. I'll reference this PR when I file the issues.

[silverwind]

I don't think we need quote-props, it's only useful in ES3 environments.

[targos]

So we just remove no-reserved-keys ? I'm fine with that.

[silverwind]

Patch doesn't seem to apply cleanly for me:

error: tools/eslint/node_modules/debug/node_modules/ms/History.md: already exists in working directory

Did you install the new eslint with NODE_ENV=production? I forgot that when updating the last time, so it might be best to git rm / git add everything in tools/eslint now.

[targos]

I did this:

rm -rf tools/eslint npm install eslint mv node_modules/eslint tools/eslint rm -rf node_modules 

[silverwind]

So we just remove no-reserved-keys ? I'm fine with that.

Yeah. You could experiment with these options thought:

quote-props: [2, "as-needed"]quote-props: [2, "consistent-as-needed"]

[silverwind]

Also, please remove escape and unescape from the globals, they're not needed anymore.

[targos]

I have one remaining issue with the indent rule and I don't know if it's on our side or eslint's:

exec('python -c "print 200000*\'C\'"',{maxBuffer: 1000},function(err,stdout,stderr){assert.ok(err);assert.ok(/maxBuffer/.test(err.message));});

 5:8 error Expected indentation of 8 characters but found 7 indent 6:8 error Expected indentation of 8 characters but found 7 indent 7:7 error Expected indentation of 6 characters but found 5 indent 

[targos]

Also, please remove escape and unescape from the globals, they're not needed anymore.

done

[silverwind]

I have one remaining

If that's the only case, I'd say refactor to a function expression (const cb = function(){... }).

[silverwind]

npm install eslint

You could make that NODE_ENV=production npm install eslint so devDependencies don't get pulled. Keeps the size of our git tree low, and I don't think we're going to run ESLint unit tests and such :)

[silverwind]

Also, I'd suggest doing git rm -r tools/eslint && rm -rf tools/eslint before and git add tools/eslint/* after, so leftover files aren't kept in the tree.

[targos]

You could make that NODE_ENV=production npm install eslint so devDependencies don't get pulled.

Isn't it the default behavior ? I don't see dev deps in tools/eslint/node_modules

suggest doing git rm -r tools/eslint && rm -rf tools/eslint before and git add tools/eslint/* after

I did it like that on last update, thanks for the tip !

[silverwind]

Isn't it the default behavior ?

You're right, I think npm did at some point install devDependencies when that variable wasn't set, but it seems fine now.

[Fishrock123]

Does this replace #2205, or does #2205 now depend on this?

[targos]

#2205 depends on this.

[Fishrock123]

I'm not sure about the switch/case: thing. Do we only have non-indented version in those two tests?

[targos]

@Fishrock123

Yes those tests are the only ones with non indented switch cases.

(Do not lose too much time reviewing until eslint is fixed upstream)

[targos]

Updated to v1.1.0. A bunch of issues have been fixed but there are new ones :(

[silverwind]

Still blocked on eslint/eslint#3173 from what I followed.