From cf46df70158a4ed4c09d5c9d655c07dc6df3ff5e Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Sat, 18 Jan 2020 00:58:46 +0900 Subject: [PATCH] Breaking: description in directive comments (refs eslint/rfcs#33) (#12699) * Breaking: description in directive comments (refs eslint/rfcs#33) * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * Update tests/lib/linter/linter.js Co-Authored-By: Kai Cataldo * Update docs/user-guide/configuring.md Co-Authored-By: Kai Cataldo * update configuring.md Co-authored-by: Kai Cataldo --- docs/user-guide/configuring.md | 26 +++ lib/linter/linter.js | 18 +- tests/lib/linter/linter.js | 308 +++++++++++++++++++++++++++++++++ 3 files changed, 349 insertions(+), 3 deletions(-) diff --git a/docs/user-guide/configuring.md b/docs/user-guide/configuring.md index 6771287d363..93cd5f4c882 100644 --- a/docs/user-guide/configuring.md +++ b/docs/user-guide/configuring.md @@ -420,6 +420,25 @@ If a rule has additional options, you can specify them using array literal synta This comment specifies the "double" option for the [`quotes`](../rules/quotes) rule. The first item in the array is always the rule severity (number or string). +Configuration comments can include descriptions to explain why the comment is necessary. The description must occur after the configuration and is separated from the configuration by two or more consecutive `-` characters. For example: + +```js +/* eslint eqeqeq: "off", curly: "error" -- Here's a description about why this configuration is necessary. */ +``` + +```js +/* eslint eqeqeq: "off", curly: "error" + -------- + Here's a description about why this configuration is necessary. */ +``` + +```js +/* eslint eqeqeq: "off", curly: "error" + * -------- + * This will not work due to the line above starting with a '*' character. + */ +``` + ### Using Configuration Files To configure rules inside of a configuration file, use the `rules` key along with an error level and any options you want to use. For example: @@ -574,6 +593,13 @@ foo(); // eslint-disable-line example/rule-name foo(); /* eslint-disable-line example/rule-name */ ``` +Configuration comments can include descriptions to explain why the comment is necessary. The description must occur after the configuration and is separated from the configuration by two or more consecutive `-` characters. For example: + +```js +// eslint-disable-next-line no-console -- Here's a description about why this configuration is necessary. +console.log('hello'); +``` + **Note:** Comments that disable warnings for a portion of a file tell ESLint not to report rule violations for the disabled code. ESLint still parses the entire file, however, so disabled code still needs to be syntactically valid JavaScript. diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 6d88cb5aa12..76d35b49eef 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -267,6 +267,15 @@ function createDisableDirectives(options) { return result; } +/** + * Remove the ignored part from a given directive comment and trim it. + * @param {string} value The comment text to strip. + * @returns {string} The stripped text. + */ +function stripDirectiveComment(value) { + return value.split(/\s-{2,}\s/u)[0].trim(); +} + /** * Parses comments in file to extract file-specific config of rules, globals * and environments and merges them with global config; also code blocks @@ -286,7 +295,7 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) { const disableDirectives = []; ast.comments.filter(token => token.type !== "Shebang").forEach(comment => { - const trimmedCommentText = comment.value.trim(); + const trimmedCommentText = stripDirectiveComment(comment.value); const match = /^(eslint(?:-env|-enable|-disable(?:(?:-next)?-line)?)?|exported|globals?)(?:\s|$)/u.exec(trimmedCommentText); if (!match) { @@ -444,8 +453,11 @@ function findEslintEnv(text) { eslintEnvPattern.lastIndex = 0; - while ((match = eslintEnvPattern.exec(text))) { - retv = Object.assign(retv || {}, commentParser.parseListConfig(match[1])); + while ((match = eslintEnvPattern.exec(text)) !== null) { + retv = Object.assign( + retv || {}, + commentParser.parseListConfig(stripDirectiveComment(match[1])) + ); } return retv; diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index e444aea920c..71bfd262a79 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -3759,6 +3759,314 @@ describe("Linter", () => { linter.verify("x", { rules: { "foo-bar-baz": "error" } }); assert(spy.calledOnce); }); + + describe("descriptions in directive comments", () => { + it("should ignore the part preceded by '--' in '/*eslint*/'.", () => { + const aaa = sinon.stub().returns({}); + const bbb = sinon.stub().returns({}); + + linter.defineRule("aaa", { create: aaa }); + linter.defineRule("bbb", { create: bbb }); + const messages = linter.verify(` + /*eslint aaa:error -- bbb:error */ + console.log("hello") + `, {}); + + // Don't include syntax error of the comment. + assert.deepStrictEqual(messages, []); + + // Use only `aaa`. + assert.strictEqual(aaa.callCount, 1); + assert.strictEqual(bbb.callCount, 0); + }); + + it("should ignore the part preceded by '--' in '/*eslint-env*/'.", () => { + const messages = linter.verify(` + /*eslint-env es2015 -- es2017 */ + var Promise = {} + var Atomics = {} + `, { rules: { "no-redeclare": "error" } }); + + // Don't include `Atomics` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endColumn: 32, + endLine: 3, + line: 3, + message: "'Promise' is already defined as a built-in global variable.", + messageId: "redeclaredAsBuiltin", + nodeType: "Identifier", + ruleId: "no-redeclare", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*global*/'.", () => { + const messages = linter.verify(` + /*global aaa -- bbb */ + var aaa = {} + var bbb = {} + `, { rules: { "no-redeclare": "error" } }); + + // Don't include `bbb` + assert.deepStrictEqual( + messages, + [{ + column: 30, + line: 2, + message: "'aaa' is already defined by a variable declaration.", + messageId: "redeclaredBySyntax", + nodeType: "Block", + ruleId: "no-redeclare", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*globals*/'.", () => { + const messages = linter.verify(` + /*globals aaa -- bbb */ + var aaa = {} + var bbb = {} + `, { rules: { "no-redeclare": "error" } }); + + // Don't include `bbb` + assert.deepStrictEqual( + messages, + [{ + column: 31, + line: 2, + message: "'aaa' is already defined by a variable declaration.", + messageId: "redeclaredBySyntax", + nodeType: "Block", + ruleId: "no-redeclare", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*exported*/'.", () => { + const messages = linter.verify(` + /*exported aaa -- bbb */ + var aaa = {} + var bbb = {} + `, { rules: { "no-unused-vars": "error" } }); + + // Don't include `aaa` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endColumn: 28, + endLine: 4, + line: 4, + message: "'bbb' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*eslint-disable*/'.", () => { + const messages = linter.verify(` + /*eslint-disable no-redeclare -- no-unused-vars */ + var aaa = {} + var aaa = {} + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-unused-vars` but not `no-redeclare` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 3, + endColumn: 28, + line: 3, + message: "'aaa' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*eslint-enable*/'.", () => { + const messages = linter.verify(` + /*eslint-disable no-redeclare, no-unused-vars */ + /*eslint-enable no-redeclare -- no-unused-vars */ + var aaa = {} + var aaa = {} + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-redeclare` but not `no-unused-vars` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 5, + endColumn: 28, + line: 5, + message: "'aaa' is already defined.", + messageId: "redeclared", + nodeType: "Identifier", + ruleId: "no-redeclare", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '//eslint-disable-line'.", () => { + const messages = linter.verify(` + var aaa = {} //eslint-disable-line no-redeclare -- no-unused-vars + var aaa = {} //eslint-disable-line no-redeclare -- no-unused-vars + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-unused-vars` but not `no-redeclare` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 2, + endColumn: 28, + line: 2, + message: "'aaa' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*eslint-disable-line*/'.", () => { + const messages = linter.verify(` + var aaa = {} /*eslint-disable-line no-redeclare -- no-unused-vars */ + var aaa = {} /*eslint-disable-line no-redeclare -- no-unused-vars */ + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-unused-vars` but not `no-redeclare` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 2, + endColumn: 28, + line: 2, + message: "'aaa' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '//eslint-disable-next-line'.", () => { + const messages = linter.verify(` + //eslint-disable-next-line no-redeclare -- no-unused-vars + var aaa = {} + //eslint-disable-next-line no-redeclare -- no-unused-vars + var aaa = {} + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-unused-vars` but not `no-redeclare` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 3, + endColumn: 28, + line: 3, + message: "'aaa' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should ignore the part preceded by '--' in '/*eslint-disable-next-line*/'.", () => { + const messages = linter.verify(` + /*eslint-disable-next-line no-redeclare -- no-unused-vars */ + var aaa = {} + /*eslint-disable-next-line no-redeclare -- no-unused-vars */ + var aaa = {} + `, { rules: { "no-redeclare": "error", "no-unused-vars": "error" } }); + + // Do include `no-unused-vars` but not `no-redeclare` + assert.deepStrictEqual( + messages, + [{ + column: 25, + endLine: 3, + endColumn: 28, + line: 3, + message: "'aaa' is assigned a value but never used.", + nodeType: "Identifier", + ruleId: "no-unused-vars", + severity: 2 + }] + ); + }); + + it("should not ignore the part preceded by '--' if the '--' is not surrounded by whitespaces.", () => { + const rule = sinon.stub().returns({}); + + linter.defineRule("a--rule", { create: rule }); + const messages = linter.verify(` + /*eslint a--rule:error */ + console.log("hello") + `, {}); + + // Don't include syntax error of the comment. + assert.deepStrictEqual(messages, []); + + // Use `a--rule`. + assert.strictEqual(rule.callCount, 1); + }); + + it("should ignore the part preceded by '--' even if the '--' is longer than 2.", () => { + const aaa = sinon.stub().returns({}); + const bbb = sinon.stub().returns({}); + + linter.defineRule("aaa", { create: aaa }); + linter.defineRule("bbb", { create: bbb }); + const messages = linter.verify(` + /*eslint aaa:error -------- bbb:error */ + console.log("hello") + `, {}); + + // Don't include syntax error of the comment. + assert.deepStrictEqual(messages, []); + + // Use only `aaa`. + assert.strictEqual(aaa.callCount, 1); + assert.strictEqual(bbb.callCount, 0); + }); + + it("should ignore the part preceded by '--' with line breaks.", () => { + const aaa = sinon.stub().returns({}); + const bbb = sinon.stub().returns({}); + + linter.defineRule("aaa", { create: aaa }); + linter.defineRule("bbb", { create: bbb }); + const messages = linter.verify(` + /*eslint aaa:error + -------- + bbb:error */ + console.log("hello") + `, {}); + + // Don't include syntax error of the comment. + assert.deepStrictEqual(messages, []); + + // Use only `aaa`. + assert.strictEqual(aaa.callCount, 1); + assert.strictEqual(bbb.callCount, 0); + }); + }); }); describe("context.getScope()", () => {