From 0e860d0b43392a2408d25c65d5de97dc132f5f37 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Wed, 1 Nov 2017 03:22:31 +0530 Subject: [PATCH 1/4] tools: custom eslint autofix for inspector-check.js 1. Adds fixer method 2. Extends test 3. Fixes merge issue Refs: #16636 --- test/parallel/test-eslint-inspector-check.js | 10 +++++++-- tools/eslint-rules/inspector-check.js | 18 ++++++++++++++-- tools/eslint-rules/rules-utils.js | 22 ++++++++++++++------ 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-eslint-inspector-check.js b/test/parallel/test-eslint-inspector-check.js index ab8314b63818c7..c3d291f41f45c1 100644 --- a/test/parallel/test-eslint-inspector-check.js +++ b/test/parallel/test-eslint-inspector-check.js @@ -12,12 +12,18 @@ const message = 'Please add a skipIfInspectorDisabled() call to allow this ' + new RuleTester().run('inspector-check', rule, { valid: [ 'foo;', - 'common.skipIfInspectorDisabled(); require("inspector");' + 'common.skipIfInspectorDisabled(); require("inspector");', + `require("common"); + common.skipIfInspectorDisabled(); + require("inspector");` ], invalid: [ { code: 'require("inspector")', - errors: [{ message }] + errors: [{ message }], + output: 'require("common");\n' + + 'common.skipIfInspectorDisabled();\n' + + 'require("inspector");' } ] }); diff --git a/tools/eslint-rules/inspector-check.js b/tools/eslint-rules/inspector-check.js index bb40a98183250c..d4f14335ad3025 100644 --- a/tools/eslint-rules/inspector-check.js +++ b/tools/eslint-rules/inspector-check.js @@ -11,16 +11,21 @@ const utils = require('./rules-utils.js'); // Rule Definition //------------------------------------------------------------------------------ const msg = 'Please add a skipIfInspectorDisabled() call to allow this ' + - 'test to be skippped when Node is built \'--without-inspector\'.'; + 'test to be skippped when Node is built \'--without-inspector\'.'; module.exports = function(context) { const missingCheckNodes = []; + const commonModuleNodes = []; var hasInspectorCheck = false; function testInspectorUsage(context, node) { if (utils.isRequired(node, ['inspector'])) { missingCheckNodes.push(node); } + + if (utils.isCommonModule(node)) { + commonModuleNodes.push(node); + } } function checkMemberExpression(context, node) { @@ -32,7 +37,16 @@ module.exports = function(context) { function reportIfMissing(context) { if (!hasInspectorCheck) { missingCheckNodes.forEach((node) => { - context.report(node, msg); + context.report({ + node, + message: msg, + fix: (fixer) => { + return fixer.insertTextAfter( + commonModuleNodes[0], + '\ncommon.skipIfInspectorDisabled();' + ); + } + }); }); } } diff --git a/tools/eslint-rules/rules-utils.js b/tools/eslint-rules/rules-utils.js index 2bfab1c6399ee8..da184a806d0afc 100644 --- a/tools/eslint-rules/rules-utils.js +++ b/tools/eslint-rules/rules-utils.js @@ -24,6 +24,16 @@ module.exports.isBinding = function(node, modules) { } }; +/** + * Return true if common module is required + * in AST Node under inspection + */ +var commonModuleRegExp = new RegExp(/^(\.\.\/)*common(\.js)?$/); +module.exports.isCommonModule = function(node) { + return node.callee.name === 'require' && + commonModuleRegExp.test(node.arguments[0].value); +}; + /** * Returns true is the node accesses any property in the properties * array on the 'common' object. @@ -45,8 +55,8 @@ module.exports.usesCommonProperty = function(node, properties) { module.exports.inSkipBlock = function(node) { var hasSkipBlock = false; if (node.test && - node.test.type === 'UnaryExpression' && - node.test.operator === '!') { + node.test.type === 'UnaryExpression' && + node.test.operator === '!') { const consequent = node.consequent; if (consequent.body) { consequent.body.some(function(expressionStatement) { @@ -64,8 +74,8 @@ module.exports.inSkipBlock = function(node) { function hasSkip(expression) { return expression && - expression.callee && - (expression.callee.name === 'skip' || - expression.callee.property && - expression.callee.property.name === 'skip'); + expression.callee && + (expression.callee.name === 'skip' || + expression.callee.property && + expression.callee.property.name === 'skip'); } From 4afc45d792876f3f0cc62373cb6f720d14aedb6e Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sun, 4 Feb 2018 17:38:41 +0530 Subject: [PATCH 2/4] tools: custom eslint autofix for inspector-check.js 1. Removes extra indentation 2. Removes array to track commomModule AST node. 3. Refactored test Refs: #16636 --- test/parallel/test-eslint-inspector-check.js | 8 ++++---- tools/eslint-rules/inspector-check.js | 16 +++++++++------- tools/eslint-rules/rules-utils.js | 12 ++++++------ 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-eslint-inspector-check.js b/test/parallel/test-eslint-inspector-check.js index c3d291f41f45c1..f8621e532ed7cc 100644 --- a/test/parallel/test-eslint-inspector-check.js +++ b/test/parallel/test-eslint-inspector-check.js @@ -13,13 +13,13 @@ new RuleTester().run('inspector-check', rule, { valid: [ 'foo;', 'common.skipIfInspectorDisabled(); require("inspector");', - `require("common"); - common.skipIfInspectorDisabled(); - require("inspector");` + 'require("common");\n' + + 'common.skipIfInspectorDisabled();\n' + + 'require("inspector");' ], invalid: [ { - code: 'require("inspector")', + code: 'require("inspector");', errors: [{ message }], output: 'require("common");\n' + 'common.skipIfInspectorDisabled();\n' + diff --git a/tools/eslint-rules/inspector-check.js b/tools/eslint-rules/inspector-check.js index d4f14335ad3025..00a2dd02963558 100644 --- a/tools/eslint-rules/inspector-check.js +++ b/tools/eslint-rules/inspector-check.js @@ -11,11 +11,11 @@ const utils = require('./rules-utils.js'); // Rule Definition //------------------------------------------------------------------------------ const msg = 'Please add a skipIfInspectorDisabled() call to allow this ' + - 'test to be skippped when Node is built \'--without-inspector\'.'; + 'test to be skippped when Node is built \'--without-inspector\'.'; module.exports = function(context) { const missingCheckNodes = []; - const commonModuleNodes = []; + var commonModuleNode = null; var hasInspectorCheck = false; function testInspectorUsage(context, node) { @@ -24,7 +24,7 @@ module.exports = function(context) { } if (utils.isCommonModule(node)) { - commonModuleNodes.push(node); + commonModuleNode = node; } } @@ -41,10 +41,12 @@ module.exports = function(context) { node, message: msg, fix: (fixer) => { - return fixer.insertTextAfter( - commonModuleNodes[0], - '\ncommon.skipIfInspectorDisabled();' - ); + if (commonModuleNode) { + return fixer.insertTextAfter( + commonModuleNode, + '\ncommon.skipIfInspectorDisabled();' + ); + } } }); }); diff --git a/tools/eslint-rules/rules-utils.js b/tools/eslint-rules/rules-utils.js index da184a806d0afc..46d753f481eab2 100644 --- a/tools/eslint-rules/rules-utils.js +++ b/tools/eslint-rules/rules-utils.js @@ -55,8 +55,8 @@ module.exports.usesCommonProperty = function(node, properties) { module.exports.inSkipBlock = function(node) { var hasSkipBlock = false; if (node.test && - node.test.type === 'UnaryExpression' && - node.test.operator === '!') { + node.test.type === 'UnaryExpression' && + node.test.operator === '!') { const consequent = node.consequent; if (consequent.body) { consequent.body.some(function(expressionStatement) { @@ -74,8 +74,8 @@ module.exports.inSkipBlock = function(node) { function hasSkip(expression) { return expression && - expression.callee && - (expression.callee.name === 'skip' || - expression.callee.property && - expression.callee.property.name === 'skip'); + expression.callee && + (expression.callee.name === 'skip' || + expression.callee.property && + expression.callee.property.name === 'skip'); } From c4b5e8b1752007abb8ba8fa3822125248d88df6f Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Thu, 15 Feb 2018 15:43:30 +0530 Subject: [PATCH 3/4] tools: custom eslint autofix for inspector-check.js Review comments + updates tests to work with fixer Refs: #16636 --- test/parallel/test-eslint-inspector-check.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-eslint-inspector-check.js b/test/parallel/test-eslint-inspector-check.js index f8621e532ed7cc..00e6025ec4f323 100644 --- a/test/parallel/test-eslint-inspector-check.js +++ b/test/parallel/test-eslint-inspector-check.js @@ -12,18 +12,18 @@ const message = 'Please add a skipIfInspectorDisabled() call to allow this ' + new RuleTester().run('inspector-check', rule, { valid: [ 'foo;', - 'common.skipIfInspectorDisabled(); require("inspector");', - 'require("common");\n' + + 'require("common")\n' + 'common.skipIfInspectorDisabled();\n' + - 'require("inspector");' + 'require("inspector")' ], invalid: [ { - code: 'require("inspector");', + code: 'require("common")\n' + + 'require("inspector")', errors: [{ message }], - output: 'require("common");\n' + + output: 'require("common")\n' + 'common.skipIfInspectorDisabled();\n' + - 'require("inspector");' + 'require("inspector")' } ] }); From 3b1ce161ebab2c16c5a097fb3429f39590efe37f Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sat, 17 Feb 2018 10:29:12 +0530 Subject: [PATCH 4/4] tools: custom eslint autofix for inspector-check.js rules-utils arguments check + spacing in test file Refs: #16636 --- test/parallel/test-eslint-inspector-check.js | 4 ++-- tools/eslint-rules/rules-utils.js | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-eslint-inspector-check.js b/test/parallel/test-eslint-inspector-check.js index 00e6025ec4f323..bdec596f8d128e 100644 --- a/test/parallel/test-eslint-inspector-check.js +++ b/test/parallel/test-eslint-inspector-check.js @@ -13,8 +13,8 @@ new RuleTester().run('inspector-check', rule, { valid: [ 'foo;', 'require("common")\n' + - 'common.skipIfInspectorDisabled();\n' + - 'require("inspector")' + 'common.skipIfInspectorDisabled();\n' + + 'require("inspector")' ], invalid: [ { diff --git a/tools/eslint-rules/rules-utils.js b/tools/eslint-rules/rules-utils.js index 46d753f481eab2..f00a833d0c0251 100644 --- a/tools/eslint-rules/rules-utils.js +++ b/tools/eslint-rules/rules-utils.js @@ -31,6 +31,7 @@ module.exports.isBinding = function(node, modules) { var commonModuleRegExp = new RegExp(/^(\.\.\/)*common(\.js)?$/); module.exports.isCommonModule = function(node) { return node.callee.name === 'require' && + node.arguments.length !== 0 && commonModuleRegExp.test(node.arguments[0].value); };