diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index 8bf97c6450f..8c9db815c26 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/docs/developer-guide/working-with-rules.md @@ -181,6 +181,63 @@ Note that leading and trailing whitespace is optional in message parameters. The node contains all of the information necessary to figure out the line and column number of the offending text as well the source text representing the node. +### `messageId`s + +Instead of typing out messages in both the `context.report()` call and your tests, you can use `messageId`s instead. + +This allows you to avoid retyping error messages. It also prevents errors reported in different sections of your rule from having out-of-date messages. + +```js +// in your rule +module.exports = { + meta: { + messages: { + avoidName: "Avoid using variables named '{{ name }}'" + } + }, + create(context) { + return { + Identifier(node) { + if (node.name === "foo") { + context.report({ + node, + messageId: "avoidName", + data: { + name: "foo", + }, + }); + } + } + }; + } +}; + +// in the file to lint: + +var foo = 2 +// ^ error: Avoid using variables named 'foo' + +// In your tests: +var rule = require('../../../lib/rules/no-insecure-random') +var RuleTester = require('eslint').RuleTester + +var ruleTester = new RuleTester() +ruleTester.run('my-rule', rule, { + valid: ['bar', 'baz'], + + invalid: [ + { + code: 'foo', + errors: [ + { + messageId: 'foo', + }, + ], + }, + ], +}) +``` + ### Applying Fixes If you'd like ESLint to attempt to fix the problem you're reporting, you can do so by specifying the `fix` function when using `context.report()`. The `fix` function receives a single argument, a `fixer` object, that you can use to apply a fix. For example: diff --git a/lib/linter.js b/lib/linter.js index 1907f148a45..2238c5be0e3 100755 --- a/lib/linter.js +++ b/lib/linter.js @@ -911,6 +911,7 @@ module.exports = class Linter { } const rule = this.rules.get(ruleId); + const messageIds = rule && rule.meta && rule.meta.messages; let reportTranslator = null; const ruleContext = Object.freeze( Object.assign( @@ -931,7 +932,7 @@ module.exports = class Linter { * with Node 8.4.0. */ if (reportTranslator === null) { - reportTranslator = createReportTranslator({ ruleId, severity, sourceCode }); + reportTranslator = createReportTranslator({ ruleId, severity, sourceCode, messageIds }); } const problem = reportTranslator.apply(null, arguments); diff --git a/lib/report-translator.js b/lib/report-translator.js index 9c4ab8b9a9f..8e5b587f96c 100644 --- a/lib/report-translator.js +++ b/lib/report-translator.js @@ -11,6 +11,7 @@ const assert = require("assert"); const ruleFixer = require("./util/rule-fixer"); +const interpolate = require("./util/interpolate"); //------------------------------------------------------------------------------ // Typedefs @@ -41,7 +42,9 @@ function normalizeMultiArgReportCall() { // If there is one argument, it is considered to be a new-style call already. if (arguments.length === 1) { - return arguments[0]; + + // Shallow clone the object to avoid surprises if reusing the descriptor + return Object.assign({}, arguments[0]); } // If the second argument is a string, the arguments are interpreted as [node, message, data, fix]. @@ -100,16 +103,7 @@ function normalizeReportLoc(descriptor) { * @returns {string} The interpolated message for the descriptor */ function normalizeMessagePlaceholders(descriptor) { - if (!descriptor.data) { - return descriptor.message; - } - return descriptor.message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => { - if (term in descriptor.data) { - return descriptor.data[term]; - } - - return fullMatch; - }); + return interpolate(descriptor.message, descriptor.data); } /** @@ -216,6 +210,14 @@ function createProblem(options) { source: options.sourceLines[options.loc.start.line - 1] || "" }; + /* + * If this isn’t in the conditional, some of the tests fail + * because `messageId` is present in the problem object + */ + if (options.messageId) { + problem.messageId = options.messageId; + } + if (options.loc.end) { problem.endLine = options.loc.end.line; problem.endColumn = options.loc.end.column + 1; @@ -231,12 +233,13 @@ function createProblem(options) { /** * Returns a function that converts the arguments of a `context.report` call from a rule into a reported * problem for the Node.js API. - * @param {{ruleId: string, severity: number, sourceCode: SourceCode}} metadata Metadata for the reported problem + * @param {{ruleId: string, severity: number, sourceCode: SourceCode, messageIds: Object}} metadata Metadata for the reported problem * @param {SourceCode} sourceCode The `SourceCode` instance for the text being linted * @returns {function(...args): { * ruleId: string, * severity: (0|1|2), - * message: string, + * message: (string|undefined), + * messageId: (string|undefined), * line: number, * column: number, * endLine: (number|undefined), @@ -261,11 +264,29 @@ module.exports = function createReportTranslator(metadata) { assertValidNodeInfo(descriptor); + if (descriptor.messageId) { + if (!metadata.messageIds) { + throw new TypeError("context.report() called with a messageId, but no messages were present in the rule metadata."); + } + const id = descriptor.messageId; + const messages = metadata.messageIds; + + if (descriptor.message) { + throw new TypeError("context.report() called with a message and a messageId. Please only pass one."); + } + if (!messages || !Object.prototype.hasOwnProperty.call(messages, id)) { + throw new TypeError(`context.report() called with a messageId of '${id}' which is not present in the 'messages' config: ${JSON.stringify(messages, null, 2)}`); + } + descriptor.message = messages[id]; + } + + return createProblem({ ruleId: metadata.ruleId, severity: metadata.severity, node: descriptor.node, message: normalizeMessagePlaceholders(descriptor), + messageId: descriptor.messageId, loc: normalizeReportLoc(descriptor), fix: normalizeFixes(descriptor, metadata.sourceCode), sourceLines: metadata.sourceCode.lines diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js index 7e69c961935..de218a875f5 100644 --- a/lib/testers/rule-tester.js +++ b/lib/testers/rule-tester.js @@ -47,7 +47,8 @@ const lodash = require("lodash"), ajv = require("../util/ajv"), Linter = require("../linter"), Environments = require("../config/environments"), - SourceCodeFixer = require("../util/source-code-fixer"); + SourceCodeFixer = require("../util/source-code-fixer"), + interpolate = require("../util/interpolate"); //------------------------------------------------------------------------------ // Private Members @@ -472,59 +473,73 @@ class RuleTester { const hasMessageOfThisRule = messages.some(m => m.ruleId === ruleName); for (let i = 0, l = item.errors.length; i < l; i++) { - assert(!messages[i].fatal, `A fatal parsing error occurred: ${messages[i].message}`); + const error = item.errors[i]; + const message = messages[i]; + + assert(!message.fatal, `A fatal parsing error occurred: ${message.message}`); assert(hasMessageOfThisRule, "Error rule name should be the same as the name of the rule being tested"); - if (typeof item.errors[i] === "string" || item.errors[i] instanceof RegExp) { + if (typeof error === "string" || error instanceof RegExp) { // Just an error message. - assertMessageMatches(messages[i].message, item.errors[i]); - } else if (typeof item.errors[i] === "object") { + assertMessageMatches(message.message, error); + } else if (typeof error === "object") { /* * Error object. * This may have a message, node type, line, and/or * column. */ - if (item.errors[i].message) { - assertMessageMatches(messages[i].message, item.errors[i].message); + if (error.message) { + assertMessageMatches(message.message, error.message); } - // The following checks use loose equality assertions for backwards compatibility. + if (error.messageId) { + const hOP = Object.hasOwnProperty.call.bind(Object.hasOwnProperty); - if (item.errors[i].type) { + // verify that `error.message` is `undefined` + assert.strictEqual(error.message, void 0, "Error should not specify both a message and a messageId."); + if (!hOP(rule, "meta") || !hOP(rule.meta, "messages")) { + assert.fail("Rule must specify a messages hash in `meta`"); + } + if (!hOP(rule.meta.messages, error.messageId)) { + const friendlyIDList = `[${Object.keys(rule.meta.messages).map(key => `'${key}'`).join(", ")}]`; - // eslint-disable-next-line no-restricted-properties - assert.equal(messages[i].nodeType, item.errors[i].type, `Error type should be ${item.errors[i].type}, found ${messages[i].nodeType}`); - } + assert.fail(`Invalid messageId '${error.messageId}'. Expected one of ${friendlyIDList}.`); + } - if (item.errors[i].hasOwnProperty("line")) { + let expectedMessage = rule.meta.messages[error.messageId]; - // eslint-disable-next-line no-restricted-properties - assert.equal(messages[i].line, item.errors[i].line, `Error line should be ${item.errors[i].line}`); - } + if (error.data) { + expectedMessage = interpolate(expectedMessage, error.data); + } - if (item.errors[i].hasOwnProperty("column")) { + assertMessageMatches(message.message, expectedMessage); + } - // eslint-disable-next-line no-restricted-properties - assert.equal(messages[i].column, item.errors[i].column, `Error column should be ${item.errors[i].column}`); + if (error.type) { + assert.strictEqual(message.nodeType, error.type, `Error type should be ${error.type}, found ${message.nodeType}`); } - if (item.errors[i].hasOwnProperty("endLine")) { + if (error.hasOwnProperty("line")) { + assert.strictEqual(message.line, error.line, `Error line should be ${error.line}`); + } - // eslint-disable-next-line no-restricted-properties - assert.equal(messages[i].endLine, item.errors[i].endLine, `Error endLine should be ${item.errors[i].endLine}`); + if (error.hasOwnProperty("column")) { + assert.strictEqual(message.column, error.column, `Error column should be ${error.column}`); } - if (item.errors[i].hasOwnProperty("endColumn")) { + if (error.hasOwnProperty("endLine")) { + assert.strictEqual(message.endLine, error.endLine, `Error endLine should be ${error.endLine}`); + } - // eslint-disable-next-line no-restricted-properties - assert.equal(messages[i].endColumn, item.errors[i].endColumn, `Error endColumn should be ${item.errors[i].endColumn}`); + if (error.hasOwnProperty("endColumn")) { + assert.strictEqual(message.endColumn, error.endColumn, `Error endColumn should be ${error.endColumn}`); } } else { // Message was an unexpected type - assert.fail(messages[i], null, "Error should be a string, object, or RegExp."); + assert.fail(message, null, "Error should be a string, object, or RegExp."); } } } diff --git a/lib/util/interpolate.js b/lib/util/interpolate.js new file mode 100644 index 00000000000..e0f2d027d19 --- /dev/null +++ b/lib/util/interpolate.js @@ -0,0 +1,24 @@ +/** + * @fileoverview Interpolate keys from an object into a string with {{ }} markers. + * @author Jed Fox + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Public Interface +//------------------------------------------------------------------------------ + +module.exports = (text, data) => { + if (!data) { + return text; + } + return text.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => { + if (term in data) { + return data[term]; + } + + // Preserve old behavior: If parameter name not provided, don't replace it. + return fullMatch; + }); +}; diff --git a/tests/lib/report-translator.js b/tests/lib/report-translator.js index e80e22c819a..f1e00ed3da5 100644 --- a/tests/lib/report-translator.js +++ b/tests/lib/report-translator.js @@ -48,7 +48,7 @@ describe("createReportTranslator", () => { node = sourceCode.ast.body[0]; location = sourceCode.ast.body[1].loc.start; message = "foo"; - translateReport = createReportTranslator({ ruleId: "foo-rule", severity: 2, sourceCode }); + translateReport = createReportTranslator({ ruleId: "foo-rule", severity: 2, sourceCode, messageIds: { testMessage: message } }); }); describe("old-style call with location", () => { @@ -113,6 +113,61 @@ describe("createReportTranslator", () => { } ); }); + it("should translate the messageId into a message", () => { + const reportDescriptor = { + node, + loc: location, + messageId: "testMessage", + fix: () => ({ range: [1, 2], text: "foo" }) + }; + + assert.deepStrictEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + messageId: "testMessage", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [1, 2], + text: "foo" + } + } + ); + }); + it("should throw when both messageId and message are provided", () => { + const reportDescriptor = { + node, + loc: location, + messageId: "testMessage", + message: "bar", + fix: () => ({ range: [1, 2], text: "foo" }) + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + "context.report() called with a message and a messageId. Please only pass one." + ); + }); + it("should throw when an invalid messageId is provided", () => { + const reportDescriptor = { + node, + loc: location, + messageId: "thisIsNotASpecifiedMessageId", + fix: () => ({ range: [1, 2], text: "foo" }) + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + /^context\.report\(\) called with a messageId of '[^']+' which is not present in the 'messages' config:/ + ); + }); }); describe("combining autofixes", () => { it("should merge fixes to one if 'fix' function returns an array of fixes.", () => { diff --git a/tests/lib/util/interpolate.js b/tests/lib/util/interpolate.js new file mode 100644 index 00000000000..89e5498db5f --- /dev/null +++ b/tests/lib/util/interpolate.js @@ -0,0 +1,22 @@ +"use strict"; + +const assert = require("chai").assert; +const interpolate = require("../../../lib/util/interpolate"); + +describe("interpolate()", () => { + it("passes through text without {{ }}", () => { + const message = "This is a very important message!"; + + assert.strictEqual(interpolate(message, {}), message); + }); + it("passes through text with {{ }} that don’t match a key", () => { + const message = "This is a very important {{ message }}!"; + + assert.strictEqual(interpolate(message, {}), message); + }); + it("Properly interpolates keys in {{ }}", () => { + assert.strictEqual(interpolate("This is a very important {{ message }}!", { + message: "test" + }), "This is a very important test!"); + }); +});