From 228518a5400939175bfd4ffda379a6bd29cdf359 Mon Sep 17 00:00:00 2001 From: Will Douglas Date: Mon, 11 Nov 2019 15:18:58 -0700 Subject: [PATCH] Add support for suggestion messageId-based descriptions --- docs/developer-guide/unit-tests.md | 4 +- docs/developer-guide/working-with-rules.md | 52 ++++++++- lib/linter/report-translator.js | 61 ++++++++-- lib/shared/types.js | 9 +- tests/lib/linter/linter.js | 44 ++++++++ tests/lib/linter/report-translator.js | 125 ++++++++++++++++++++- 6 files changed, 276 insertions(+), 19 deletions(-) diff --git a/docs/developer-guide/unit-tests.md b/docs/developer-guide/unit-tests.md index 9b9bd440744a..281c39674540 100644 --- a/docs/developer-guide/unit-tests.md +++ b/docs/developer-guide/unit-tests.md @@ -12,10 +12,10 @@ This automatically starts Mocha and runs all tests in the `tests` directory. You If you want to quickly run just one test, you can do so by running Mocha directly and passing in the filename. For example: - npm test:cli tests/lib/rules/no-wrap-func.js + npm run test:cli tests/lib/rules/no-wrap-func.js Running individual tests is useful when you're working on a specific bug and iterating on the solution. You should be sure to run `npm test` before submitting a pull request. ## More Control on Unit Testing -`npm test:cli` is an alias of the Mocha cli in `./node_modules/.bin/mocha`. [Options](https://mochajs.org/#command-line-usage) are available to be provided to help to better control the test to run. +`npm run test:cli` is an alias of the Mocha cli in `./node_modules/.bin/mocha`. [Options](https://mochajs.org/#command-line-usage) are available to be provided to help to better control the test to run. diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index 97303dcab33e..2e0aa83ec84b 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/docs/developer-guide/working-with-rules.md @@ -349,17 +349,24 @@ Best practices for fixes: In some cases fixes aren't appropriate to be automatically applied, for example, if a fix potentially changes functionality or if there are multiple valid ways to fix a rule depending on the implementation intent (see the best practices for [applying fixes](#applying-fixes) listed above). In these cases, there is an alternative `suggest` option on `context.report()` that allows other tools, such as editors, to expose helpers for users to manually apply a suggestion. -In order to provide suggestions, use the `suggest` key in the report argument with an array of suggestion objects. The suggestion objects represent individual suggestions that could be applied and require a `desc` key string that describes what applying the suggestion would do, and a `fix` key that is a function defining the suggestion result. This `fix` function follows the same API as regular fixes (described above in [applying fixes](#applying-fixes)). +In order to provide suggestions, use the `suggest` key in the report argument with an array of suggestion objects. The suggestion objects represent individual suggestions that could be applied and require either a `desc` key string that describes what applying the suggestion would do or a `messageId` key (see [below](#suggestion-messageids)), and a `fix` key that is a function defining the suggestion result. This `fix` function follows the same API as regular fixes (described above in [applying fixes](#applying-fixes)). ```js context.report({ node: node, - message: "Missing semicolon", + message: "Unnecessary escape character: \\{{character}}.", + data: { character }, suggest: [ { - desc: "Insert the missing semicolon", + desc: "Remove unnecessary escape.", fix: function(fixer) { - return fixer.insertTextAfter(node, ";"); + return fixer.removeRange(range); + } + }, + { + desc: "Escape backslash to include it in the RegExp.", + fix: function(fixer) { + return fixer.insertTextBeforeRange(range, "\\"); } } ] @@ -368,6 +375,43 @@ context.report({ Note: Suggestions will be applied as a stand-alone change, without triggering multipass fixes. Each suggestion should focus on a singular change in the code and should not try to confirm to user defined styles. For example, if a suggestion is adding a new statement into the codebase, it should not try to match correct indentation, or confirm to user preferences on presence/absence of semicolumns. All of those things can be corrected by multipass autofix when the user triggers it. +#### Suggestion `messageId`s + +Instead of using a `desc` key for suggestions a `messageId` can be used instead. This works the same way as `messageId`s for the overall error (see [messageIds](#messageIds)). Here is an example of how to use it in a rule: + +```js +module.exports = { + meta: { + messages: { + removeEscape: "Remove unnecessary escape.", + escapeBackslash: "Escape backslash to include it in the RegExp." + } + }, + create: function(context) { + // ... + context.report({ + node: node, + message: "Unnecessary escape character: \\{{character}}.", + data: { character }, + suggest: [ + { + messageId: "removeEscape", + fix: function(fixer) { + return fixer.removeRange(range); + } + }, + { + messageId: "escapeBackslash", + fix: function(fixer) { + return fixer.insertTextBeforeRange(range, "\\"); + } + } + ] + }); + } +}; +``` + ### context.options Some rules require options in order to function correctly. These options appear in configuration (`.eslintrc`, command line, or in comments). For example: diff --git a/lib/linter/report-translator.js b/lib/linter/report-translator.js index 874bb4aef7b2..eef5165585b2 100644 --- a/lib/linter/report-translator.js +++ b/lib/linter/report-translator.js @@ -26,7 +26,7 @@ const interpolate = require("./interpolate"); * @property {Object} [data] Optional data to use to fill in placeholders in the * message. * @property {Function} [fix] The function to call that creates a fix command. - * @property {Array<{desc: string, fix: Function}>} suggest Suggestion descriptions and functions to create a the associated fixes. + * @property {Array<{desc?: string, messageId?: string, fix: Function}>} suggest Suggestion descriptions and functions to create a the associated fixes. */ /** @@ -188,17 +188,23 @@ function normalizeFixes(descriptor, sourceCode) { * Gets an array of suggestion objects from the given descriptor. * @param {MessageDescriptor} descriptor The report descriptor. * @param {SourceCode} sourceCode The source code object to get text between fixes. - * @returns {Array<{text: string, range: number[]}>} The suggestions for the descriptor + * @param {Object} messages Object of meta messages for the rule. + * @returns {Array} The suggestions for the descriptor */ -function mapSuggestions(descriptor, sourceCode) { +function mapSuggestions(descriptor, sourceCode, messages) { if (!descriptor.suggest || !Array.isArray(descriptor.suggest)) { return []; } - return descriptor.suggest.map(suggestInfo => ({ - desc: suggestInfo.desc, - fix: normalizeFixes(suggestInfo, sourceCode) - })); + return descriptor.suggest.map(suggestInfo => { + const computedDesc = suggestInfo.desc || messages[suggestInfo.messageId]; + + return { + ...suggestInfo, + desc: interpolate(computedDesc, suggestInfo.data), + fix: normalizeFixes(suggestInfo, sourceCode) + }; + }); } /** @@ -248,6 +254,40 @@ function createProblem(options) { return problem; } +/** + * Validates that suggestions are properly defined. Throws if an error is detected. + * @param {Array<{ desc?: string, messageId?: string }>} suggest The incoming suggest data. + * @param {Object} messages Object of meta messages for the rule. + * @returns {void} + */ +function validateSuggestions(suggest, messages) { + if (suggest && Array.isArray(suggest)) { + suggest.forEach(suggestion => { + if (suggestion.messageId) { + const { messageId } = suggestion; + + if (!messages) { + throw new TypeError(`context.report() called with a suggest option with a messageId '${messageId}', but no messages were present in the rule metadata.`); + } + + if (!messages[messageId]) { + throw new TypeError(`context.report() called with a suggest option with a messageId '${messageId}' which is not present in the 'messages' config: ${JSON.stringify(messages, null, 2)}`); + } + + if (suggestion.desc) { + throw new TypeError("context.report() called with a suggest option that defines both a 'messageId' and an 'desc'. Please only pass one."); + } + } else if (!suggestion.desc) { + throw new TypeError("context.report() called with a suggest option that doesn't have either a `desc` or `messageId`"); + } + + if (typeof suggestion.fix !== "function") { + throw new TypeError(`context.report() called with a suggest option without a fix function. See: ${suggestion}`); + } + }); + } +} + /** * Returns a function that converts the arguments of a `context.report` call from a rule into a reported * problem for the Node.js API. @@ -266,17 +306,17 @@ module.exports = function createReportTranslator(metadata) { */ return (...args) => { const descriptor = normalizeMultiArgReportCall(...args); + const messages = metadata.messageIds; assertValidNodeInfo(descriptor); let computedMessage; if (descriptor.messageId) { - if (!metadata.messageIds) { + if (!messages) { 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."); @@ -291,6 +331,7 @@ module.exports = function createReportTranslator(metadata) { throw new TypeError("Missing `message` property in report() call; add a message that describes the linting problem."); } + validateSuggestions(descriptor.suggest, messages); return createProblem({ ruleId: metadata.ruleId, @@ -300,7 +341,7 @@ module.exports = function createReportTranslator(metadata) { messageId: descriptor.messageId, loc: normalizeReportLoc(descriptor), fix: metadata.disableFixes ? null : normalizeFixes(descriptor, metadata.sourceCode), - suggestions: metadata.disableFixes ? [] : mapSuggestions(descriptor, metadata.sourceCode) + suggestions: metadata.disableFixes ? [] : mapSuggestions(descriptor, metadata.sourceCode, messages) }); }; }; diff --git a/lib/shared/types.js b/lib/shared/types.js index 94de725d8599..e200a9ddd95d 100644 --- a/lib/shared/types.js +++ b/lib/shared/types.js @@ -91,7 +91,14 @@ module.exports = {}; * @property {string} message The error message. * @property {string|null} ruleId The ID of the rule which makes this message. * @property {0|1|2} severity The severity of this message. - * @property {Array<{desc: string, fix: {range: [number, number], text: string}}>} [suggestions] Information for suggestions. + * @property {Array<{desc?: string, messageId?: string, fix: {range: [number, number], text: string}}>} [suggestions] Information for suggestions. + */ + +/** + * @typedef {Object} SuggestionResult + * @property {string} [desc] A short description. Required unless `messageId` is defined. + * @property {string} [messageId] Id referencing a message for the description. + * @property {{ text: string, range: number[] }} fix fix result info */ /** diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index c8f80d1cd170..859c414a9f89 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -4385,6 +4385,50 @@ describe("Linter", () => { } }]); }); + + it("supports messageIds for suggestions", () => { + linter.defineRule("rule-with-suggestions", { + meta: { + messages: { + suggestion1: "Insert space at the beginning", + suggestion2: "Insert space at the end" + } + }, + create: context => ({ + Program(node) { + context.report({ + node, + message: "Incorrect spacing", + suggest: [{ + messageId: "suggestion1", + fix: fixer => fixer.insertTextBefore(node, " ") + }, { + messageId: "suggestion2", + fix: fixer => fixer.insertTextAfter(node, " ") + }] + }); + } + }) + }); + + const messages = linter.verify("var a = 1;", { rules: { "rule-with-suggestions": "error" } }); + + assert.deepStrictEqual(messages[0].suggestions, [{ + messageId: "suggestion1", + desc: "Insert space at the beginning", + fix: { + range: [0, 0], + text: " " + } + }, { + messageId: "suggestion2", + desc: "Insert space at the end", + fix: { + range: [10, 10], + text: " " + } + }]); + }); }); describe("mutability", () => { diff --git a/tests/lib/linter/report-translator.js b/tests/lib/linter/report-translator.js index 542a8af4b4fa..a79400cf7e40 100644 --- a/tests/lib/linter/report-translator.js +++ b/tests/lib/linter/report-translator.js @@ -40,7 +40,7 @@ describe("createReportTranslator", () => { ); } - let node, location, message, translateReport; + let node, location, message, translateReport, suggestion1, suggestion2; beforeEach(() => { const sourceCode = createSourceCode("foo\nbar"); @@ -48,7 +48,18 @@ describe("createReportTranslator", () => { node = sourceCode.ast.body[0]; location = sourceCode.ast.body[1].loc.start; message = "foo"; - translateReport = createReportTranslator({ ruleId: "foo-rule", severity: 2, sourceCode, messageIds: { testMessage: message } }); + suggestion1 = "First suggestion"; + suggestion2 = "Second suggestion {{interpolated}}"; + translateReport = createReportTranslator({ + ruleId: "foo-rule", + severity: 2, + sourceCode, + messageIds: { + testMessage: message, + suggestion1, + suggestion2 + } + }); }); describe("old-style call with location", () => { @@ -191,6 +202,116 @@ describe("createReportTranslator", () => { "Missing `message` property in report() call; add a message that describes the linting problem." ); }); + + it("should support messageIds for suggestions and output resulting descriptions", () => { + const reportDescriptor = { + node, + loc: location, + message, + suggest: [{ + messageId: "suggestion1", + fix: () => ({ range: [2, 3], text: "s1" }) + }, { + messageId: "suggestion2", + data: { interpolated: "'interpolated value'" }, + fix: () => ({ range: [3, 4], text: "s2" }) + }] + }; + + assert.deepStrictEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + suggestions: [{ + messageId: "suggestion1", + desc: "First suggestion", + fix: { range: [2, 3], text: "s1" } + }, { + messageId: "suggestion2", + data: { interpolated: "'interpolated value'" }, + desc: "Second suggestion 'interpolated value'", + fix: { range: [3, 4], text: "s2" } + }] + } + ); + }); + + it("should throw when a suggestion defines both a desc and messageId", () => { + const reportDescriptor = { + node, + loc: location, + message, + suggest: [{ + desc: "The description", + messageId: "suggestion1", + fix: () => ({ range: [2, 3], text: "s1" }) + }] + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + "context.report() called with a suggest option that defines both a 'messageId' and an 'desc'. Please only pass one." + ); + }); + + it("should throw when a suggestion uses an invalid messageId", () => { + const reportDescriptor = { + node, + loc: location, + message, + suggest: [{ + messageId: "noMatchingMessage", + fix: () => ({ range: [2, 3], text: "s1" }) + }] + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + /^context\.report\(\) called with a suggest option with a messageId '[^']+' which is not present in the 'messages' config:/u + ); + }); + + it("should throw when a suggestion does not provide either a desc or messageId", () => { + const reportDescriptor = { + node, + loc: location, + message, + suggest: [{ + fix: () => ({ range: [2, 3], text: "s1" }) + }] + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + "context.report() called with a suggest option that doesn't have either a `desc` or `messageId`" + ); + }); + + it("should throw when a suggestion does not provide a fix function", () => { + const reportDescriptor = { + node, + loc: location, + message, + suggest: [{ + desc: "The description", + fix: false + }] + }; + + assert.throws( + () => translateReport(reportDescriptor), + TypeError, + /^context\.report\(\) called with a suggest option without a fix function. See:/u + ); + }); }); describe("combining autofixes", () => {