From 509b3b81d68095cee8a0c732b527072bc9537260 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 16 Oct 2017 18:37:14 -0400 Subject: [PATCH] tools: enable additional eslint rules Enable additional rules that node either already adheres to or it makes sense to do so going forward: for-direction, accessor-pairs, no-lonely-if and symbol-description. Fix all instances of no-lonely-if in lib & test and disable accessor-pairs in test-util-inspect. PR-URL: https://github.com/nodejs/node/pull/16243 Refs: https://eslint.org/docs/rules/for-direction Refs: https://eslint.org/docs/rules/accessor-pairs Refs: https://eslint.org/docs/rules/no-lonely-if Refs: https://eslint.org/docs/rules/symbol-description Reviewed-By: Ruben Bridgewater Reviewed-By: Vse Mozhet Byt Reviewed-By: Refael Ackermann Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig --- .eslintrc.yaml | 4 +++ doc/.eslintrc.yaml | 1 + lib/_http_incoming.js | 5 ++-- lib/_http_outgoing.js | 24 ++++++++--------- lib/_http_server.js | 14 +++++----- lib/_stream_readable.js | 25 +++++++++--------- lib/_tls_legacy.js | 4 +-- lib/child_process.js | 14 +++++----- lib/events.js | 10 +++---- lib/fs.js | 24 ++++++++--------- lib/internal/http2/core.js | 11 ++++---- lib/path.js | 18 ++++++------- lib/querystring.js | 5 ++-- lib/url.js | 16 +++++------- test/.eslintrc.yaml | 1 + test/abort/test-abort-uncaught-exception.js | 7 +++-- .../test-cluster-disconnect-unshared-tcp.js | 8 +++--- .../test-cluster-disconnect-unshared-udp.js | 8 +++--- test/parallel/test-cluster-worker-destroy.js | 22 +++++++--------- test/parallel/test-tls-server-verify.js | 26 +++++++++---------- test/parallel/test-util-inspect.js | 3 +++ tools/eslint-rules/rules-utils.js | 6 ++--- 22 files changed, 118 insertions(+), 138 deletions(-) diff --git a/.eslintrc.yaml b/.eslintrc.yaml index d8768459c2e77c..e24830d9923bbe 100644 --- a/.eslintrc.yaml +++ b/.eslintrc.yaml @@ -18,6 +18,7 @@ overrides: rules: # Possible Errors # http://eslint.org/docs/rules/#possible-errors + for-direction: error no-control-regex: error no-debugger: error no-dupe-args: error @@ -41,6 +42,7 @@ rules: # Best Practices # http://eslint.org/docs/rules/#best-practices + accessor-pairs: error dot-location: [error, property] eqeqeq: [error, smart] no-fallthrough: error @@ -122,6 +124,7 @@ rules: ignoreUrls: true, tabWidth: 2}] new-parens: error + no-lonely-if: error no-mixed-spaces-and-tabs: error no-multiple-empty-lines: [error, {max: 2, maxEOF: 0, maxBOF: 0}] no-restricted-syntax: [error, { @@ -172,6 +175,7 @@ rules: no-this-before-super: error prefer-const: [error, {ignoreReadBeforeAssign: true}] rest-spread-spacing: error + symbol-description: error template-curly-spacing: error # Custom rules in tools/eslint-rules diff --git a/doc/.eslintrc.yaml b/doc/.eslintrc.yaml index b269eb14e625f4..8038836fe310db 100644 --- a/doc/.eslintrc.yaml +++ b/doc/.eslintrc.yaml @@ -6,6 +6,7 @@ rules: no-undef: off no-unused-vars: off strict: off + symbol-description: off # add new ECMAScript features gradually no-var: error diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js index 6e5aff1cc9c1f2..696fcc3b4ce53d 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -301,10 +301,9 @@ function _addHeaderLine(field, value, dest) { } else { dest['set-cookie'] = [value]; } - } else { + } else if (dest[field] === undefined) { // Drop duplicates - if (dest[field] === undefined) - dest[field] = value; + dest[field] = value; } } diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index c484ac46819a70..ebc94ba95335b5 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -404,20 +404,18 @@ function _storeHeader(firstLine, headers) { this.chunkedEncoding = false; } else if (!this.useChunkedEncodingByDefault) { this._last = true; + } else if (!state.trailer && + !this._removedContLen && + typeof this._contentLength === 'number') { + state.header += 'Content-Length: ' + this._contentLength + CRLF; + } else if (!this._removedTE) { + state.header += 'Transfer-Encoding: chunked\r\n'; + this.chunkedEncoding = true; } else { - if (!state.trailer && - !this._removedContLen && - typeof this._contentLength === 'number') { - state.header += 'Content-Length: ' + this._contentLength + CRLF; - } else if (!this._removedTE) { - state.header += 'Transfer-Encoding: chunked\r\n'; - this.chunkedEncoding = true; - } else { - // We should only be able to get here if both Content-Length and - // Transfer-Encoding are removed by the user. - // See: test/parallel/test-http-remove-header-stays-removed.js - debug('Both Content-Length and Transfer-Encoding are removed'); - } + // We should only be able to get here if both Content-Length and + // Transfer-Encoding are removed by the user. + // See: test/parallel/test-http-remove-header-stays-removed.js + debug('Both Content-Length and Transfer-Encoding are removed'); } } diff --git a/lib/_http_server.js b/lib/_http_server.js index b6a5841cddc3d9..df42a7e9167f61 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -429,8 +429,8 @@ function socketOnEnd(server, socket, parser, state) { state.outgoing[state.outgoing.length - 1]._last = true; } else if (socket._httpMessage) { socket._httpMessage._last = true; - } else { - if (socket.writable) socket.end(); + } else if (socket.writable) { + socket.end(); } } @@ -590,13 +590,11 @@ function parserOnIncoming(server, socket, state, req, keepAlive) { res.writeContinue(); server.emit('request', req, res); } + } else if (server.listenerCount('checkExpectation') > 0) { + server.emit('checkExpectation', req, res); } else { - if (server.listenerCount('checkExpectation') > 0) { - server.emit('checkExpectation', req, res); - } else { - res.writeHead(417); - res.end(); - } + res.writeHead(417); + res.end(); } } else { server.emit('request', req, res); diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 123a4f3558e103..2a69942f394e33 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -40,20 +40,19 @@ const kProxyEvents = ['error', 'close', 'destroy', 'pause', 'resume']; function prependListener(emitter, event, fn) { // Sadly this is not cacheable as some libraries bundle their own // event emitter implementation with them. - if (typeof emitter.prependListener === 'function') { + if (typeof emitter.prependListener === 'function') return emitter.prependListener(event, fn); - } else { - // This is a hack to make sure that our error handler is attached before any - // userland ones. NEVER DO THIS. This is here only because this code needs - // to continue to work with older versions of Node.js that do not include - // the prependListener() method. The goal is to eventually remove this hack. - if (!emitter._events || !emitter._events[event]) - emitter.on(event, fn); - else if (Array.isArray(emitter._events[event])) - emitter._events[event].unshift(fn); - else - emitter._events[event] = [fn, emitter._events[event]]; - } + + // This is a hack to make sure that our error handler is attached before any + // userland ones. NEVER DO THIS. This is here only because this code needs + // to continue to work with older versions of Node.js that do not include + // the prependListener() method. The goal is to eventually remove this hack. + if (!emitter._events || !emitter._events[event]) + emitter.on(event, fn); + else if (Array.isArray(emitter._events[event])) + emitter._events[event].unshift(fn); + else + emitter._events[event] = [fn, emitter._events[event]]; } function ReadableState(options, stream) { diff --git a/lib/_tls_legacy.js b/lib/_tls_legacy.js index b25be2d6a3b444..745a55889993fb 100644 --- a/lib/_tls_legacy.js +++ b/lib/_tls_legacy.js @@ -878,8 +878,8 @@ SecurePair.prototype.error = function(returnOnly) { /peer did not return a certificate/.test(err.message)) { // Not really an error. this.destroy(); - } else { - if (!returnOnly) this.cleartext.emit('error', err); + } else if (!returnOnly) { + this.cleartext.emit('error', err); } return err; }; diff --git a/lib/child_process.js b/lib/child_process.js index c951718c52e8e1..fa3eeebdab4e34 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -323,11 +323,10 @@ exports.execFile = function(file /*, args, options, callback*/) { if (stdoutLen > options.maxBuffer) { ex = new Error('stdout maxBuffer exceeded'); kill(); + } else if (encoding) { + _stdout += chunk; } else { - if (encoding) - _stdout += chunk; - else - _stdout.push(chunk); + _stdout.push(chunk); } }); } @@ -342,11 +341,10 @@ exports.execFile = function(file /*, args, options, callback*/) { if (stderrLen > options.maxBuffer) { ex = new Error('stderr maxBuffer exceeded'); kill(); + } else if (encoding) { + _stderr += chunk; } else { - if (encoding) - _stderr += chunk; - else - _stderr.push(chunk); + _stderr.push(chunk); } }); } diff --git a/lib/events.js b/lib/events.js index dce1dc48fa42e8..d615fc2132223f 100644 --- a/lib/events.js +++ b/lib/events.js @@ -264,13 +264,11 @@ function _addListener(target, type, listener, prepend) { // Adding the second element, need to change to array. existing = events[type] = prepend ? [listener, existing] : [existing, listener]; - } else { // If we've already got an array, just append. - if (prepend) { - existing.unshift(listener); - } else { - existing.push(listener); - } + } else if (prepend) { + existing.unshift(listener); + } else { + existing.push(listener); } // Check for listener leak diff --git a/lib/fs.js b/lib/fs.js index 28d3d1ff513cab..8959aa4800195d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1241,21 +1241,19 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback) { callback(writeErr); }); } - } else { - if (written === length) { - if (isUserFd) { - callback(null); - } else { - fs.close(fd, callback); - } + } else if (written === length) { + if (isUserFd) { + callback(null); } else { - offset += written; - length -= written; - if (position !== null) { - position += written; - } - writeAll(fd, isUserFd, buffer, offset, length, position, callback); + fs.close(fd, callback); + } + } else { + offset += written; + length -= written; + if (position !== null) { + position += written; } + writeAll(fd, isUserFd, buffer, offset, length, position, callback); } }); } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 83db79b950e5c1..66dc28c46fb333 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -187,12 +187,11 @@ function onSessionHeaders(id, cat, flags, headers) { } } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) { event = 'push'; - } else { // cat === NGHTTP2_HCAT_HEADERS: - if (!endOfStream && status !== undefined && status >= 200) { - event = 'response'; - } else { - event = endOfStream ? 'trailers' : 'headers'; - } + // cat === NGHTTP2_HCAT_HEADERS: + } else if (!endOfStream && status !== undefined && status >= 200) { + event = 'response'; + } else { + event = endOfStream ? 'trailers' : 'headers'; } debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`); process.nextTick(emit, stream, event, obj, flags, headers); diff --git a/lib/path.js b/lib/path.js index bd147bf2b45bff..a7b95c42ac55ee 100644 --- a/lib/path.js +++ b/lib/path.js @@ -455,17 +455,15 @@ const win32 = { } else { return ''; } + } else if (isAbsolute) { + if (tail.length > 0) + return device + '\\' + tail; + else + return device + '\\'; + } else if (tail.length > 0) { + return device + tail; } else { - if (isAbsolute) { - if (tail.length > 0) - return device + '\\' + tail; - else - return device + '\\'; - } else if (tail.length > 0) { - return device + tail; - } else { - return device; - } + return device; } }, diff --git a/lib/querystring.js b/lib/querystring.js index b4851f57c3e4e0..936b7463c9e5f6 100644 --- a/lib/querystring.js +++ b/lib/querystring.js @@ -315,9 +315,8 @@ function parse(qs, sep, eq, options) { sepIdx = eqIdx = 0; continue; } - } else { - if (lastPos < end) - value += qs.slice(lastPos, end); + } else if (lastPos < end) { + value += qs.slice(lastPos, end); } if (key.length > 0 && keyEncoded) diff --git a/lib/url.js b/lib/url.js index f8f21c3ffcb6d1..052fb3cd469ed9 100644 --- a/lib/url.js +++ b/lib/url.js @@ -126,16 +126,14 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) { if (isWs) continue; lastPos = start = i; - } else { - if (inWs) { - if (!isWs) { - end = -1; - inWs = false; - } - } else if (isWs) { - end = i; - inWs = true; + } else if (inWs) { + if (!isWs) { + end = -1; + inWs = false; } + } else if (isWs) { + end = i; + inWs = true; } // Only convert backslashes while we haven't seen a split character diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index 58e072c1951e27..6be0ca4e3f83bc 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -5,6 +5,7 @@ rules: # http://eslint.org/docs/rules/#ecmascript-6 no-var: error prefer-const: error + symbol-description: off # Custom rules in tools/eslint-rules prefer-assert-iferror: error diff --git a/test/abort/test-abort-uncaught-exception.js b/test/abort/test-abort-uncaught-exception.js index e88a56a48954c6..4acddf349c227a 100644 --- a/test/abort/test-abort-uncaught-exception.js +++ b/test/abort/test-abort-uncaught-exception.js @@ -24,11 +24,10 @@ function run(flags, signals) { assert.strictEqual(code, 0xC0000005); else assert.strictEqual(code, 1); + } else if (signals) { + assert(signals.includes(sig), `Unexpected signal ${sig}`); } else { - if (signals) - assert(signals.includes(sig), `Unexpected signal ${sig}`); - else - assert.strictEqual(sig, null); + assert.strictEqual(sig, null); } })); } diff --git a/test/parallel/test-cluster-disconnect-unshared-tcp.js b/test/parallel/test-cluster-disconnect-unshared-tcp.js index 59df0c54795ec2..e18617788c53f6 100644 --- a/test/parallel/test-cluster-disconnect-unshared-tcp.js +++ b/test/parallel/test-cluster-disconnect-unshared-tcp.js @@ -37,10 +37,8 @@ if (cluster.isMaster) { unbound.disconnect(); unbound.on('disconnect', cluster.disconnect); } -} else { - if (process.env.BOUND === 'y') { - const source = net.createServer(); +} else if (process.env.BOUND === 'y') { + const source = net.createServer(); - source.listen(0); - } + source.listen(0); } diff --git a/test/parallel/test-cluster-disconnect-unshared-udp.js b/test/parallel/test-cluster-disconnect-unshared-udp.js index b0670f3e323671..5857e9132f9679 100644 --- a/test/parallel/test-cluster-disconnect-unshared-udp.js +++ b/test/parallel/test-cluster-disconnect-unshared-udp.js @@ -40,10 +40,8 @@ if (cluster.isMaster) { unbound.disconnect(); unbound.on('disconnect', cluster.disconnect); } -} else { - if (process.env.BOUND === 'y') { - const source = dgram.createSocket('udp4'); +} else if (process.env.BOUND === 'y') { + const source = dgram.createSocket('udp4'); - source.bind(0); - } + source.bind(0); } diff --git a/test/parallel/test-cluster-worker-destroy.js b/test/parallel/test-cluster-worker-destroy.js index e4223b826d8fbe..65b41381f569bb 100644 --- a/test/parallel/test-cluster-worker-destroy.js +++ b/test/parallel/test-cluster-worker-destroy.js @@ -41,17 +41,15 @@ if (cluster.isMaster) { worker.on('disconnect', common.mustCall()); worker.on('exit', common.mustCall()); }); -} else { - if (cluster.worker.id === 1) { - // Call destroy when worker is disconnected - cluster.worker.process.on('disconnect', function() { - cluster.worker.destroy(); - }); - - const w = cluster.worker.disconnect(); - assert.strictEqual(w, cluster.worker); - } else { - // Call destroy when worker is not disconnected yet +} else if (cluster.worker.id === 1) { + // Call destroy when worker is disconnected + cluster.worker.process.on('disconnect', function() { cluster.worker.destroy(); - } + }); + + const w = cluster.worker.disconnect(); + assert.strictEqual(w, cluster.worker); +} else { + // Call destroy when worker is not disconnected yet + cluster.worker.destroy(); } diff --git a/test/parallel/test-tls-server-verify.js b/test/parallel/test-tls-server-verify.js index f9075539c69a51..69378aab94e4b3 100644 --- a/test/parallel/test-tls-server-verify.js +++ b/test/parallel/test-tls-server-verify.js @@ -335,21 +335,19 @@ function runTest(port, testIndex) { port = server.address().port; if (tcase.debug) { console.error(`${prefix}TLS server running on port ${port}`); + } else if (tcase.renegotiate) { + runNextClient(0); } else { - if (tcase.renegotiate) { - runNextClient(0); - } else { - let clientsCompleted = 0; - for (let i = 0; i < tcase.clients.length; i++) { - runClient(`${prefix}${i} `, port, tcase.clients[i], function() { - clientsCompleted++; - if (clientsCompleted === tcase.clients.length) { - server.close(); - successfulTests++; - runTest(port, nextTest++); - } - }); - } + let clientsCompleted = 0; + for (let i = 0; i < tcase.clients.length; i++) { + runClient(`${prefix}${i} `, port, tcase.clients[i], function() { + clientsCompleted++; + if (clientsCompleted === tcase.clients.length) { + server.close(); + successfulTests++; + runTest(port, nextTest++); + } + }); } } }); diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js index 733a6843009e52..722ae7d4bd9723 100644 --- a/test/parallel/test-util-inspect.js +++ b/test/parallel/test-util-inspect.js @@ -26,6 +26,8 @@ const JSStream = process.binding('js_stream').JSStream; const util = require('util'); const vm = require('vm'); +/* eslint-disable accessor-pairs */ + assert.strictEqual(util.inspect(1), '1'); assert.strictEqual(util.inspect(false), 'false'); assert.strictEqual(util.inspect(''), "''"); @@ -1124,3 +1126,4 @@ if (typeof Symbol !== 'undefined') { } assert.doesNotThrow(() => util.inspect(process)); +/* eslint-enable accessor-pairs */ diff --git a/tools/eslint-rules/rules-utils.js b/tools/eslint-rules/rules-utils.js index e3e5e6e5ef9718..f2f5428ed1cbc1 100644 --- a/tools/eslint-rules/rules-utils.js +++ b/tools/eslint-rules/rules-utils.js @@ -43,10 +43,8 @@ module.exports.inSkipBlock = function(node) { } return false; }); - } else { - if (hasSkip(consequent.expression)) { - hasSkipBlock = true; - } + } else if (hasSkip(consequent.expression)) { + hasSkipBlock = true; } } return hasSkipBlock;