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

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

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.

I did this:

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

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"]

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

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

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

done

I have one remaining

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

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 :)

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.

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 !

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.

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

#2205 depends on this.

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

@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)

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

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

The linter went from 45 seconds to 1min 18sec for a run.

Not too bad, I guess. Eslint is under heavy development after all and they have to take care of a lot of edge cases and ES6 additions.

@targos two Windows failures, not sure if flaky. The first test was modified here, the second was not:

not ok 73 test-fs-realpath.js
# TIMEOUT
# start tests
# fs.symlinkSync("c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\fixtures/nested-index/one", "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\tmp.0/symlink", "dir")
# test_simple_relative_symlink
# fs.symlinkSync("../tmp.0/cycles/root.js", "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\0\\VS_VERSION\\vs2013\\label\\win2008r2\\test\\tmp.0/symlink", "file")
# test_simple_absolute_symlink
# using type=dir
# test_deep_relative_file_symlink
# test_deep_relative_dir_symlink
# test_cyclic_link_protection

not ok 171 test-require-long-path.js
# internal/module.js:42
# if (content.charCodeAt(0) === 0xFEFF) {
# ^
# 
# TypeError: Cannot read property 'charCodeAt' of undefined
# at Object.stripBOM (internal/module.js:42:14)
# at Object.Module._extensions..js (module.js:432:34)
# at Module.load (module.js:367:32)
# at Function.Module._load (module.js:324:12)
# at Module.require (module.js:377:17)
# at require (internal/module.js:16:19)
# at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win10\test\parallel\test-require-long-path.js:25:1)
# at Module._compile (module.js:423:34)
# at Object.Module._extensions..js (module.js:432:10)
# at Module.load (module.js:367:32)

Ah, false alarm it looks like. These failures both are present in the previous CI run:

https://ci.nodejs.org/job/node-test-binary-windows/577/

Going to land this!

@silverwind see #4679

Landed in ed55169 .. d1aabd6. Huge thanks to @targos for persisting!

🎉

Yay!

@targos Great work man :-)

@rvagg @thealphanerd @targos @nodejs/lts ... I'm struggling on this one to decide if it should go into LTS. If we don't land it, it's likely to make backporting changes more difficult because of the altered linting, but I'm not sure how much of a difference it will make. Any thoughts?

I am inclined to think that this is not an LTS material. Quoting https://github.com/nodejs/LTS#lts-plan,

Once a release enters LTS, no new features may be added to that release. Changes are limited to bug fixes, security updates, possible npm updates, documentation updates, and certain performance improvements that can be demonstrated to not break existing applications, etc. Semver-minor changes are only permitted if required for bug fixes. Semver-major changes are only permitted if required for critical security and bug fixes.

This doesn't fall under any of the mentioned categories, right?

@thefourtheye I agree, but as we continue to evolve style rules, it'll make backporting commits a bit more difficult. I can live with it, but @thealphanerd and I could definitely use a bit more active help in getting things pulled back into staging :-)

as we continue to evolve style rules, it'll make backporting commits a bit more difficult.

Ah, right. I didn't think of that. Then +1 to backport this to LTS.

+1 to backporting as well

Yeah, I think it's necessary too, or else make test will fail when changes which adhere to the new rules land on LTS.

I'm +1 on backporting tooling where possible, it makes it easier to manage across branches and it's not really related to API or even the code we're shipping.

Proposed backport: #4721