From 7ffb22f61cf1622511a7fe42b5ead7c3b216df5e Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Mon, 7 Oct 2019 18:51:44 -0700 Subject: [PATCH] Chore: Clean up inline directive parsing (#12375) * Give variable name to matched text This simply makes the code a bit easier to read by giving a name to `match[1]`. * Refactor: Untangle logic for parsing directives There are a few thing going on in this function which were getting conflated: 1. Parsing a `directiveType` out of the comment. 2. Ignoring directives that are in line comments but only support block comments. 3. Warning on and ignoring line comments that span multiple lines. Previously these three pieces of functionality were tightly coupled which made the code harder to read. After this change each task is handled independently of the other. * Core: Consolidate handling disable directives Rather than conditionally set a mutable value and check for it at the end of the switch statement, we can actually just handle it inline by using a fallthrough. --- lib/linter/linter.js | 176 +++++++++++++++++++++---------------------- 1 file changed, 87 insertions(+), 89 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 311cd8995af..5b9ce07a50d 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -292,10 +292,15 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) { if (!match) { return; } - const lineCommentSupported = /^eslint-disable-(next-)?line$/u.test(match[1]); + const directiveText = match[1]; + const lineCommentSupported = /^eslint-disable-(next-)?line$/u.test(directiveText); - if (warnInlineConfig && (lineCommentSupported || comment.type === "Block")) { - const kind = comment.type === "Block" ? `/*${match[1]}*/` : `//${match[1]}`; + if (comment.type === "Line" && !lineCommentSupported) { + return; + } + + if (warnInlineConfig) { + const kind = comment.type === "Block" ? `/*${directiveText}*/` : `//${directiveText}`; problems.push(createLintingProblem({ ruleId: null, @@ -306,108 +311,101 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) { return; } - const directiveValue = trimmedCommentText.slice(match.index + match[1].length); - let directiveType = ""; + if (lineCommentSupported && comment.loc.start.line !== comment.loc.end.line) { + const message = `${directiveText} comment should not span multiple lines.`; - if (lineCommentSupported) { - if (comment.loc.start.line === comment.loc.end.line) { - directiveType = match[1].slice("eslint-".length); - } else { - const message = `${match[1]} comment should not span multiple lines.`; + problems.push(createLintingProblem({ + ruleId: null, + message, + loc: comment.loc + })); + return; + } - problems.push(createLintingProblem({ - ruleId: null, - message, - loc: comment.loc - })); + const directiveValue = trimmedCommentText.slice(match.index + directiveText.length); + + switch (directiveText) { + case "eslint-disable": + case "eslint-enable": + case "eslint-disable-next-line": + case "eslint-disable-line": { + const directiveType = directiveText.slice("eslint-".length); + const options = { type: directiveType, loc: comment.loc, value: directiveValue, ruleMapper }; + const { directives, directiveProblems } = createDisableDirectives(options); + + disableDirectives.push(...directives); + problems.push(...directiveProblems); + break; } - } else if (comment.type === "Block") { - switch (match[1]) { - case "exported": - Object.assign(exportedVariables, commentParser.parseStringConfig(directiveValue, comment)); - break; - case "globals": - case "global": - for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) { - let normalizedValue; + case "exported": + Object.assign(exportedVariables, commentParser.parseStringConfig(directiveValue, comment)); + break; + + case "globals": + case "global": + for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) { + let normalizedValue; + + try { + normalizedValue = ConfigOps.normalizeConfigGlobal(value); + } catch (err) { + problems.push(createLintingProblem({ + ruleId: null, + loc: comment.loc, + message: err.message + })); + continue; + } + + if (enabledGlobals[id]) { + enabledGlobals[id].comments.push(comment); + enabledGlobals[id].value = normalizedValue; + } else { + enabledGlobals[id] = { + comments: [comment], + value: normalizedValue + }; + } + } + break; + + case "eslint": { + const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); + + if (parseResult.success) { + Object.keys(parseResult.config).forEach(name => { + const rule = ruleMapper(name); + const ruleValue = parseResult.config[name]; + + if (rule === null) { + problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); + return; + } try { - normalizedValue = ConfigOps.normalizeConfigGlobal(value); + validator.validateRuleOptions(rule, name, ruleValue); } catch (err) { problems.push(createLintingProblem({ - ruleId: null, - loc: comment.loc, - message: err.message + ruleId: name, + message: err.message, + loc: comment.loc })); - continue; - } - if (enabledGlobals[id]) { - enabledGlobals[id].comments.push(comment); - enabledGlobals[id].value = normalizedValue; - } else { - enabledGlobals[id] = { - comments: [comment], - value: normalizedValue - }; + // do not apply the config, if found invalid options. + return; } - } - break; - - case "eslint-disable": - directiveType = "disable"; - break; - - case "eslint-enable": - directiveType = "enable"; - break; - - case "eslint": { - const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); - - if (parseResult.success) { - Object.keys(parseResult.config).forEach(name => { - const rule = ruleMapper(name); - const ruleValue = parseResult.config[name]; - - if (rule === null) { - problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); - return; - } - - try { - validator.validateRuleOptions(rule, name, ruleValue); - } catch (err) { - problems.push(createLintingProblem({ - ruleId: name, - message: err.message, - loc: comment.loc - })); - - // do not apply the config, if found invalid options. - return; - } - - configuredRules[name] = ruleValue; - }); - } else { - problems.push(parseResult.error); - } - break; + configuredRules[name] = ruleValue; + }); + } else { + problems.push(parseResult.error); } - // no default + break; } - } - - if (directiveType !== "") { - const options = { type: directiveType, loc: comment.loc, value: directiveValue, ruleMapper }; - const { directives, directiveProblems } = createDisableDirectives(options); - disableDirectives.push(...directives); - problems.push(...directiveProblems); + // no default } });