From ca2bb1e5e4d9fcb4784acd536f7df6f367bfec1e Mon Sep 17 00:00:00 2001 From: Casey Visco Date: Wed, 17 Aug 2016 21:42:11 -0400 Subject: [PATCH] Update: Refactor one-dependency-per-line rule * DRY-up code * Add `meta` information * Replace custom `unique` function with a `Set` --- lib/rules/one-dependency-per-line.js | 182 +++++++++------------ lib/util.js | 10 +- lib/utils/unique.js | 12 -- tests/lib/rules/one-dependency-per-line.js | 8 +- tests/lib/utils/unique.js | 50 ------ 5 files changed, 87 insertions(+), 175 deletions(-) delete mode 100644 lib/utils/unique.js delete mode 100644 tests/lib/utils/unique.js diff --git a/lib/rules/one-dependency-per-line.js b/lib/rules/one-dependency-per-line.js index 2092e2a..b61e589 100644 --- a/lib/rules/one-dependency-per-line.js +++ b/lib/rules/one-dependency-per-line.js @@ -1,11 +1,10 @@ /** - * @fileoverview Require or disallow one dependency per line. + * @fileoverview Rule to enforce or disallow one dependency per line. * @author Casey Visco */ "use strict"; -const unique = require("../utils/unique"); const util = require("../util"); const ast = require("../utils/ast"); @@ -15,9 +14,62 @@ const isArrayExpr = ast.isArrayExpr; const isFunctionExpr = ast.isFunctionExpr; const isStringLiteral = ast.isStringLiteral; +// ----------------------------------------------------------------------------- +// Helpers +// ----------------------------------------------------------------------------- + +const pad = (amount) => (value) => " ".repeat(amount) + value; +const line = (node) => node.loc.start.line; +const column = (node) => node.loc.start.column; +const unique = (list) => Array.from(new Set(list)); +const hasDuplicates = (list) => unique(list).length < list.length; +const hasMultiple = (list) => unique(list).length > 1; + +const indentation = (node) => { + const statement = node.body.body[0]; + return statement && line(node) !== line(statement) ? column(statement) : 0; +}; + +const formatPaths = (indent) => (node) => { + const paths = node.elements + .map(v => v.raw) + .map(pad(indent)) + .join(",\n"); + + return `[\n${paths}\n]`; +}; + +const formatNames = (indent, context) => (node) => { + const body = context.getSourceCode().getText(node.body); + const names = node.params + .map(v => v.name) + .map(pad(indent)) + .join(",\n"); + + return `function (\n${names}\n) ${body}`; +}; + +// ----------------------------------------------------------------------------- +// Rule Definition +// ----------------------------------------------------------------------------- + +const ALWAYS_MSG = { + paths: "Only one dependency path is permitted per line.", + names: "Only one dependency name is permitted per line." +}; + +const NEVER_MSG = { + paths: "Dependency paths must appear on one line.", + names: "Dependency names must appear on one line." +}; + module.exports = { meta: { - docs: {}, + docs: { + description: "Require or disallow one dependency per line", + category: "Stylistic Choices", + recommended: false + }, fixable: "code", schema: [ { @@ -52,11 +104,6 @@ module.exports = { }, create: function (context) { - const ALWAYS_PATHS_MESSAGE = "Only one dependency path is permitted per line."; - const ALWAYS_NAMES_MESSAGE = "Only one dependency name is permitted per line."; - const NEVER_PATHS_MESSAGE = "Dependency paths must appear on one line."; - const NEVER_NAMES_MESSAGE = "Dependency names must appear on one line."; - const options = context.options[0] || {}; const settings = { @@ -64,95 +111,38 @@ module.exports = { names: "names" in options ? options.names : "always" }; - const sourceCode = context.getSourceCode(); - - function lineNum(node) { - return node.loc.start.line; - } - - function hasDuplicateValues(list) { - return unique(list).length < list.length; - } - - function hasMultipleValues(list) { - return unique(list).length > 1; - } - function isAlways(setting, list) { const value = settings[setting]; - - if (value === "always") { - return true; - } - - if (value === "never") { - return false; + switch (value) { + case "always": + return true; + case "never": + return false; + default: + return list.length > value; } - - return list.length > value; } function isNever(setting) { return settings[setting] === "never"; } - function getFileIndentationLevel(args) { - const functionLine = lineNum(args[1]); - const firstStatementLine = lineNum(args[1].body.body[0]); + function check(setting, node, list, format) { + const lines = list.map(line); + const fix = (fixer) => fixer.replaceTextRange(node.range, format(node)); - if (functionLine !== firstStatementLine) { - return args[1].body.body[0].loc.start.column; + if (isAlways(setting, lines) && hasDuplicates(lines)) { + context.report({ node, fix, message: ALWAYS_MSG[setting] }); + } else if (isNever(setting) && hasMultiple(lines)) { + context.report({ node, message: NEVER_MSG[setting] }); } - - return 0; - } - - function getPathsFixer(args) { - return function (fixer) { - const indentLevel = isFunctionExpr(args[1]) ? getFileIndentationLevel(args) : 0; - const pathsNode = args[0]; - const paths = pathsNode.elements; - let formattedPaths = ""; - - paths.forEach(function (path) { - formattedPaths += " ".repeat(indentLevel) + path.raw + ",\n"; - }); - - formattedPaths = formattedPaths.slice(0, -2); - let replacerText = "[\n" + formattedPaths + "\n]"; - - return fixer.replaceTextRange(pathsNode.range, replacerText); - }; - } - - function getNamesFixer(args) { - return function (fixer) { - const indentLevel = isFunctionExpr(args[1]) ? getFileIndentationLevel(args) : 0; - const namesNode = args[1]; - const names = namesNode.params; - - let replacerText = ""; - let formattedNames = ""; - - names.forEach(function (name) { - formattedNames += " ".repeat(indentLevel) + name.name + ",\n"; - }); - - formattedNames = formattedNames.slice(0, -2); - replacerText += "function (\n" + formattedNames + "\n)"; - replacerText += " " + sourceCode.getText(namesNode.body); - - return fixer.replaceTextRange(namesNode.range, replacerText); - }; } return { "CallExpression": function (node) { let args = node.arguments; - let paths = []; - let names = []; - if (!(isDefineCall(node) || isRequireCall(node)) || args.length < 2) { + if (!isDefineCall(node) && !isRequireCall(node) || args.length < 2) { return; } @@ -161,34 +151,18 @@ module.exports = { args = args.slice(1); } - // Get dependency path list - if (isArrayExpr(args[0])) { - paths = args[0].elements; - } else if (isRequireCall(node) && isStringLiteral(args[0])) { - paths = [ args[0] ]; - } else { - return; - } + const deps = args[0]; + const func = args[1]; - // Get dependency alias list - if (isFunctionExpr(args[1])) { - names = args[1].params; + // We can only work with valid AMD-Style require or define calls + if (!isArrayExpr(deps) || !isFunctionExpr(func)) { + return; } - const pathsLineNums = paths.map(lineNum); - const namesLineNums = names.map(lineNum); + const indent = indentation(args[1]); - if (isAlways("paths", pathsLineNums) && hasDuplicateValues(pathsLineNums)) { - context.report({node: node, message: ALWAYS_PATHS_MESSAGE, fix: getPathsFixer(args, pathsLineNums)}); - } else if (isNever("paths") && hasMultipleValues(pathsLineNums)) { - context.report(node, NEVER_PATHS_MESSAGE); - } - - if (isAlways("names", namesLineNums) && hasDuplicateValues(namesLineNums)) { - context.report({node: node, message: ALWAYS_NAMES_MESSAGE, fix: getNamesFixer(args, namesLineNums)}); - } else if (isNever("names") && hasMultipleValues(namesLineNums)) { - context.report(node, NEVER_NAMES_MESSAGE); - } + check("paths", deps, deps.elements, formatPaths(indent)); + check("names", func, func.params, formatNames(indent, context)); } }; } diff --git a/lib/util.js b/lib/util.js index 20039ff..43ae413 100644 --- a/lib/util.js +++ b/lib/util.js @@ -80,7 +80,7 @@ function isDefineCall(node) { * @returns {Boolean} true if represents an AMD style module definition */ function isAmdDefine(node) { - const args = node.arguments || []; + const args = node.arguments; return args.length === 2 && isArrayExpr(args[0]) && isFunctionExpr(args[1]) || args.length === 3 && isStringLiteral(args[0]) && isArrayExpr(args[1]) && isFunctionExpr(args[2]); } @@ -97,7 +97,7 @@ function isAmdDefine(node) { * @returns {Boolean} true if represents an Object Module Definition */ function isObjectDefine(node) { - const args = node.arguments || []; + const args = node.arguments; return args.length === 1 && isObjectExpr(args[0]) || args.length === 2 && isStringLiteral(args[0]) && isObjectExpr(args[1]); } @@ -113,7 +113,7 @@ function isObjectDefine(node) { * @returns {Boolean} true if represents a Simple Function Definition */ function isFunctionDefine(node) { - const args = node.arguments || []; + const args = node.arguments; return args.length === 1 && isSimpleFuncExpr(args[0]) || args.length === 2 && isStringLiteral(args[0]) && isSimpleFuncExpr(args[1]); } @@ -129,7 +129,7 @@ function isFunctionDefine(node) { * @returns {Boolean} true if represents a Simplified CommonJS Wrapper */ function isCommonJsWrapper(node) { - const args = node.arguments || []; + const args = node.arguments; return args.length === 1 && isCommonJsFuncExpr(args[0]) || args.length === 2 && isStringLiteral(args[0]) && isCommonJsFuncExpr(args[1]); } @@ -145,7 +145,7 @@ function isCommonJsWrapper(node) { * @returns {Boolean} true if represents a named module definition function */ function isNamedDefine(node) { - const args = node.arguments || []; + const args = node.arguments; if (args.length < 2 || args.length > 3) { return false; diff --git a/lib/utils/unique.js b/lib/utils/unique.js deleted file mode 100644 index 34835da..0000000 --- a/lib/utils/unique.js +++ /dev/null @@ -1,12 +0,0 @@ -"use strict"; - -/** - * Creates a duplicate-free version of an array - * @param {Array} array - the array to inspect - * @returns {Array} duplicate-free array - */ -module.exports = function (array) { - return array.filter(function (item, i) { - return array.indexOf(item) === i; - }); -}; diff --git a/tests/lib/rules/one-dependency-per-line.js b/tests/lib/rules/one-dependency-per-line.js index 7345545..7a66274 100644 --- a/tests/lib/rules/one-dependency-per-line.js +++ b/tests/lib/rules/one-dependency-per-line.js @@ -11,22 +11,22 @@ const rule = require("../../../lib/rules/one-dependency-per-line"); const ALWAYS_PATHS_ERROR = { message: "Only one dependency path is permitted per line.", - type: "CallExpression" + type: "ArrayExpression" }; const ALWAYS_NAMES_ERROR = { message: "Only one dependency name is permitted per line.", - type: "CallExpression" + type: "FunctionExpression" }; const NEVER_PATHS_ERROR = { message: "Dependency paths must appear on one line.", - type: "CallExpression" + type: "ArrayExpression" }; const NEVER_NAMES_ERROR = { message: "Dependency names must appear on one line.", - type: "CallExpression" + type: "FunctionExpression" }; testRule("one-dependency-per-line", rule, { diff --git a/tests/lib/utils/unique.js b/tests/lib/utils/unique.js deleted file mode 100644 index 7c05762..0000000 --- a/tests/lib/utils/unique.js +++ /dev/null @@ -1,50 +0,0 @@ -"use strict"; - -const assert = require("assert"); -const unique = require("../../../lib/utils/unique"); - -describe("unique", function () { - - it("should return unique values of an unsorted array", function () { - const actual = unique([2, 1, 2]); - const expected = [2, 1]; - - assert.deepEqual(actual, expected); - }); - - it("should return unique values of an sorted array", function () { - const actual = unique([1, 2, 2]); - const expected = [1, 2]; - - assert.deepEqual(actual, expected); - }); - - it("should treat `-0` as `0`", function () { - const actual = unique([-0, 0]); - const expected = [0]; - - assert.deepEqual(actual, expected); - }); - - it("should return the same list if no duplicates are present", function () { - const actual = unique([1, 2, 3]); - const expected = [1, 2, 3]; - - assert.deepEqual(actual, expected); - }); - - it("should handle string arrays", function () { - const actual = unique(["a", "b", "a"]); - const expected = ["a", "b"]; - - assert.deepEqual(actual, expected); - }); - - it("should handle empty arrays", function () { - const actual = unique([]); - const expected = []; - - assert.deepEqual(actual, expected); - }); - -});