From adcd4201e6ff4dc6c974d4dfb0ba3c8b4286f5bc Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Sun, 26 Sep 2021 23:19:48 -0400 Subject: [PATCH] Breaking: Update `no-missing-placeholders`, `no-unused-placeholders`, `prefer-message-ids`, `prefer-placeholders` rules to also apply to suggestion messages (#196) --- lib/rules/no-missing-placeholders.js | 47 ++++++++-------- lib/rules/no-unused-placeholders.js | 57 ++++++++++--------- lib/rules/prefer-message-ids.js | 13 +++-- lib/rules/prefer-placeholders.js | 55 ++++++++++--------- lib/utils.js | 39 +++++++++++++ tests/lib/rules/no-missing-placeholders.js | 58 ++++++++++++++++++++ tests/lib/rules/no-unused-placeholders.js | 35 ++++++++++++ tests/lib/rules/prefer-message-ids.js | 19 +++++++ tests/lib/rules/prefer-placeholders.js | 64 ++++++++++++++++++++-- tests/lib/utils.js | 50 +++++++++++++++++ 10 files changed, 350 insertions(+), 87 deletions(-) diff --git a/lib/rules/no-missing-placeholders.js b/lib/rules/no-missing-placeholders.js index 77aca1b2..1f66c2bb 100644 --- a/lib/rules/no-missing-placeholders.js +++ b/lib/rules/no-missing-placeholders.js @@ -45,33 +45,36 @@ module.exports = { node.callee.property.type === 'Identifier' && node.callee.property.name === 'report' ) { const reportInfo = utils.getReportInfo(node.arguments, context); - if (!reportInfo || !reportInfo.message) { + if (!reportInfo) { return; } - const messageStaticValue = getStaticValue(reportInfo.message, context.getScope()); - if ( - ( - (reportInfo.message.type === 'Literal' && typeof reportInfo.message.value === 'string') || - (messageStaticValue && typeof messageStaticValue.value === 'string') - ) && - (!reportInfo.data || reportInfo.data.type === 'ObjectExpression') - ) { - // Same regex as the one ESLint uses - // https://github.com/eslint/eslint/blob/e5446449d93668ccbdb79d78cc69f165ce4fde07/lib/eslint.js#L990 - const PLACEHOLDER_MATCHER = /{{\s*([^{}]+?)\s*}}/g; - let match; + const reportMessagesAndDataArray = utils.collectReportViolationAndSuggestionData(reportInfo).filter(obj => obj.message); + for (const { message, data } of reportMessagesAndDataArray) { + const messageStaticValue = getStaticValue(message, context.getScope()); + if ( + ( + (message.type === 'Literal' && typeof message.value === 'string') || + (messageStaticValue && typeof messageStaticValue.value === 'string') + ) && + (!data || data.type === 'ObjectExpression') + ) { + // Same regex as the one ESLint uses + // https://github.com/eslint/eslint/blob/e5446449d93668ccbdb79d78cc69f165ce4fde07/lib/eslint.js#L990 + const PLACEHOLDER_MATCHER = /{{\s*([^{}]+?)\s*}}/g; + let match; - while ((match = PLACEHOLDER_MATCHER.exec(reportInfo.message.value || messageStaticValue.value))) { // eslint-disable-line no-extra-parens - const matchingProperty = reportInfo.data && - reportInfo.data.properties.find(prop => utils.getKeyName(prop) === match[1]); + while ((match = PLACEHOLDER_MATCHER.exec(message.value || messageStaticValue.value))) { // eslint-disable-line no-extra-parens + const matchingProperty = data && + data.properties.find(prop => utils.getKeyName(prop) === match[1]); - if (!matchingProperty) { - context.report({ - node: reportInfo.message, - messageId: 'placeholderDoesNotExist', - data: { missingKey: match[1] }, - }); + if (!matchingProperty) { + context.report({ + node: message, + messageId: 'placeholderDoesNotExist', + data: { missingKey: match[1] }, + }); + } } } } diff --git a/lib/rules/no-unused-placeholders.js b/lib/rules/no-unused-placeholders.js index 83c38a40..9cfc158d 100644 --- a/lib/rules/no-unused-placeholders.js +++ b/lib/rules/no-unused-placeholders.js @@ -45,38 +45,41 @@ module.exports = { node.callee.property.type === 'Identifier' && node.callee.property.name === 'report' ) { const reportInfo = utils.getReportInfo(node.arguments, context); - if (!reportInfo || !reportInfo.message) { + if (!reportInfo) { return; } - const messageStaticValue = getStaticValue(reportInfo.message, context.getScope()); - if ( - ( - (reportInfo.message.type === 'Literal' && typeof reportInfo.message.value === 'string') || - (messageStaticValue && typeof messageStaticValue.value === 'string') - ) && - reportInfo.data && - reportInfo.data.type === 'ObjectExpression' - ) { - const message = reportInfo.message.value || messageStaticValue.value; - // https://github.com/eslint/eslint/blob/2874d75ed8decf363006db25aac2d5f8991bd969/lib/linter.js#L986 - const PLACEHOLDER_MATCHER = /{{\s*([^{}]+?)\s*}}/g; - const placeholdersInMessage = new Set(); + const reportMessagesAndDataArray = utils.collectReportViolationAndSuggestionData(reportInfo).filter(obj => obj.message); + for (const { message, data } of reportMessagesAndDataArray) { + const messageStaticValue = getStaticValue(message, context.getScope()); + if ( + ( + (message.type === 'Literal' && typeof message.value === 'string') || + (messageStaticValue && typeof messageStaticValue.value === 'string') + ) && + data && + data.type === 'ObjectExpression' + ) { + const messageValue = message.value || messageStaticValue.value; + // https://github.com/eslint/eslint/blob/2874d75ed8decf363006db25aac2d5f8991bd969/lib/linter.js#L986 + const PLACEHOLDER_MATCHER = /{{\s*([^{}]+?)\s*}}/g; + const placeholdersInMessage = new Set(); - message.replace(PLACEHOLDER_MATCHER, (fullMatch, term) => { - placeholdersInMessage.add(term); - }); + messageValue.replace(PLACEHOLDER_MATCHER, (fullMatch, term) => { + placeholdersInMessage.add(term); + }); - reportInfo.data.properties.forEach(prop => { - const key = utils.getKeyName(prop); - if (!placeholdersInMessage.has(key)) { - context.report({ - node: reportInfo.message, - messageId: 'placeholderUnused', - data: { unusedKey: key }, - }); - } - }); + data.properties.forEach(prop => { + const key = utils.getKeyName(prop); + if (!placeholdersInMessage.has(key)) { + context.report({ + node: message, + messageId: 'placeholderUnused', + data: { unusedKey: key }, + }); + } + }); + } } } }, diff --git a/lib/rules/prefer-message-ids.js b/lib/rules/prefer-message-ids.js index 8fddd35e..e6a0dc50 100644 --- a/lib/rules/prefer-message-ids.js +++ b/lib/rules/prefer-message-ids.js @@ -68,14 +68,17 @@ module.exports = { node.callee.property.type === 'Identifier' && node.callee.property.name === 'report' ) { const reportInfo = utils.getReportInfo(node.arguments, context); - if (!reportInfo || !reportInfo.message) { + if (!reportInfo) { return; } - context.report({ - node: reportInfo.message.parent, - messageId: 'foundMessage', - }); + const reportMessagesAndDataArray = utils.collectReportViolationAndSuggestionData(reportInfo).filter(obj => obj.message); + for (const { message } of reportMessagesAndDataArray) { + context.report({ + node: message.parent, + messageId: 'foundMessage', + }); + } } }, }; diff --git a/lib/rules/prefer-placeholders.js b/lib/rules/prefer-placeholders.js index 21cf23a2..666d23b1 100644 --- a/lib/rules/prefer-placeholders.js +++ b/lib/rules/prefer-placeholders.js @@ -49,42 +49,43 @@ module.exports = { ) { const reportInfo = utils.getReportInfo(node.arguments, context); - if (!reportInfo || !reportInfo.message) { + if (!reportInfo) { return; } - let messageNode = reportInfo.message; + const reportMessagesAndDataArray = utils.collectReportViolationAndSuggestionData(reportInfo).filter(obj => obj.message); + for (let { message: messageNode } of reportMessagesAndDataArray) { + if (messageNode.type === 'Identifier') { + // See if we can find the variable declaration. - if (messageNode.type === 'Identifier') { - // See if we can find the variable declaration. + const variable = findVariable( + scopeManager.acquire(messageNode) || scopeManager.globalScope, + messageNode + ); - const variable = findVariable( - scopeManager.acquire(messageNode) || scopeManager.globalScope, - messageNode - ); + if ( + !variable || + !variable.defs || + !variable.defs[0] || + !variable.defs[0].node || + variable.defs[0].node.type !== 'VariableDeclarator' || + !variable.defs[0].node.init + ) { + return; + } + + messageNode = variable.defs[0].node.init; + } if ( - !variable || - !variable.defs || - !variable.defs[0] || - !variable.defs[0].node || - variable.defs[0].node.type !== 'VariableDeclarator' || - !variable.defs[0].node.init + (messageNode.type === 'TemplateLiteral' && messageNode.expressions.length > 0) || + (messageNode.type === 'BinaryExpression' && messageNode.operator === '+') ) { - return; + context.report({ + node: messageNode, + messageId: 'usePlaceholders', + }); } - - messageNode = variable.defs[0].node.init; - } - - if ( - (messageNode.type === 'TemplateLiteral' && messageNode.expressions.length > 0) || - (messageNode.type === 'BinaryExpression' && messageNode.operator === '+') - ) { - context.report({ - node: messageNode, - messageId: 'usePlaceholders', - }); } } }, diff --git a/lib/utils.js b/lib/utils.js index f07d4991..8ed164b4 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -169,6 +169,17 @@ function getRuleExportsCJS (ast) { }, {}); } +/** + * Find the value of a property in an object by its property key name. + * @param {Object} obj + * @param {String} keyName + * @returns property value + */ +function findObjectPropertyValueByKeyName (obj, keyName) { + const property = obj.properties.find(prop => prop.key.type === 'Identifier' && prop.key.name === keyName); + return property ? property.value : undefined; +} + module.exports = { /** * Performs static analysis on an AST to try to determine the final value of `module.exports`. @@ -385,4 +396,32 @@ module.exports = { `,\n${propertyText}` ); }, + + /** + * Collect all context.report({...}) violation/suggestion-related nodes into a standardized array for convenience. + * @param {Object} reportInfo - Result of getReportInfo(). + * @returns {messageId?: String, message?: String, data?: Object, fix?: Function}[] + */ + collectReportViolationAndSuggestionData (reportInfo) { + return [ + // Violation message + { + messageId: reportInfo.messageId, + message: reportInfo.message, + data: reportInfo.data, + fix: reportInfo.fix, + }, + // Suggestion messages + ...((reportInfo.suggest && reportInfo.suggest.elements) || []) + .map(suggestObjNode => { + return { + messageId: findObjectPropertyValueByKeyName(suggestObjNode, 'messageId'), + message: findObjectPropertyValueByKeyName(suggestObjNode, 'desc'), // Note: suggestion message named `desc` + data: findObjectPropertyValueByKeyName(suggestObjNode, 'data'), + fix: findObjectPropertyValueByKeyName(suggestObjNode, 'fix'), + }; + } + ), + ]; + }, }; diff --git a/tests/lib/rules/no-missing-placeholders.js b/tests/lib/rules/no-missing-placeholders.js index 09b8ce36..997f236f 100644 --- a/tests/lib/rules/no-missing-placeholders.js +++ b/tests/lib/rules/no-missing-placeholders.js @@ -128,6 +128,24 @@ ruleTester.run('no-missing-placeholders', rule, { context.report(node, MESSAGE, { baz: 'qux' }); }; `, + // Suggestion with placeholder + ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + desc: 'Remove {{functionName}}', + data: { + functionName: 'foo' + } + } + ] + }); + } + }; + `, ], invalid: [ @@ -212,5 +230,45 @@ ruleTester.run('no-missing-placeholders', rule, { `, errors: [error('bar')], }, + + { + // Suggestion (no `data`) + code: ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + desc: 'Remove {{bar}}' + } + ] + }); + } + }; + `, + errors: [error('bar')], + }, + { + // Suggestion (`data` but missing placeholder) + code: ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + desc: 'Remove {{bar}}', + data: { + notBar: 'abc' + } + } + ] + }); + } + }; + `, + errors: [error('bar')], + }, ], }); diff --git a/tests/lib/rules/no-unused-placeholders.js b/tests/lib/rules/no-unused-placeholders.js index 4646de9f..87b6f063 100644 --- a/tests/lib/rules/no-unused-placeholders.js +++ b/tests/lib/rules/no-unused-placeholders.js @@ -107,6 +107,22 @@ ruleTester.run('no-unused-placeholders', rule, { context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { bar: 'baz' }); }; `, + // Suggestion + ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + desc: 'foo {{bar}}', + data: { 'bar': 'baz' } + } + ] + }); + } + }; + `, ], invalid: [ @@ -168,5 +184,24 @@ ruleTester.run('no-unused-placeholders', rule, { `, errors: [error('baz')], }, + { + // Suggestion + code: ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + desc: 'foo', + data: { bar } + } + ] + }); + } + }; + `, + errors: [error('bar')], + }, ], }); diff --git a/tests/lib/rules/prefer-message-ids.js b/tests/lib/rules/prefer-message-ids.js index 2dc53436..952509f9 100644 --- a/tests/lib/rules/prefer-message-ids.js +++ b/tests/lib/rules/prefer-message-ids.js @@ -29,6 +29,14 @@ ruleTester.run('prefer-message-ids', rule, { } }; `, + // Suggestion + ` + module.exports = { + create(context) { + context.report({ node, suggest: [{messageId:'foo'}] }); + } + }; + `, { // ESM code: ` @@ -102,6 +110,17 @@ ruleTester.run('prefer-message-ids', rule, { `, errors: [{ messageId: 'foundMessage', type: 'Property' }], }, + { + // Suggestion + code: ` + module.exports = { + create(context) { + context.report({ node, suggest: [{desc:'foo'}] }); + } + }; + `, + errors: [{ messageId: 'foundMessage', type: 'Property' }], + }, { // ESM code: ` diff --git a/tests/lib/rules/prefer-placeholders.js b/tests/lib/rules/prefer-placeholders.js index 2de61b85..1bf9f401 100644 --- a/tests/lib/rules/prefer-placeholders.js +++ b/tests/lib/rules/prefer-placeholders.js @@ -17,8 +17,6 @@ const RuleTester = require('eslint').RuleTester; // ------------------------------------------------------------------------------ const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } }); -const ERROR = { messageId: 'usePlaceholders' }; - ruleTester.run('prefer-placeholders', rule, { valid: [ @@ -78,6 +76,44 @@ ruleTester.run('prefer-placeholders', rule, { } }; `, + // Suggestion (message + data) + ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { + message: '{{foo}} is bad.', + data: { foo }, + } + ] + }); + } + }; + `, + // Suggestion (message) + ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ { message: 'hello world' }] + }); + } + }; + `, + // Suggestion (messageId) + ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ { messageId: 'myMessageId' }] + }); + } + }; + `, ], invalid: [ @@ -92,7 +128,23 @@ ruleTester.run('prefer-placeholders', rule, { } }; `, - errors: [ERROR], + errors: [{ messageId: 'usePlaceholders', type: 'TemplateLiteral' }], + }, + { + // Suggestion + code: ` + module.exports = { + create(context) { + context.report({ + node, + suggest: [ + { desc: \`\${foo} is bad.\` } + ] + }); + } + }; + `, + errors: [{ messageId: 'usePlaceholders', type: 'TemplateLiteral' }], }, { // With message in variable. @@ -107,7 +159,7 @@ ruleTester.run('prefer-placeholders', rule, { } }; `, - errors: [ERROR], + errors: [{ messageId: 'usePlaceholders', type: 'TemplateLiteral' }], }, { code: ` @@ -120,7 +172,7 @@ ruleTester.run('prefer-placeholders', rule, { } }; `, - errors: [ERROR], + errors: [{ messageId: 'usePlaceholders', type: 'BinaryExpression' }], }, { code: ` @@ -130,7 +182,7 @@ ruleTester.run('prefer-placeholders', rule, { } }; `, - errors: [ERROR], + errors: [{ messageId: 'usePlaceholders', type: 'TemplateLiteral' }], }, ], }); diff --git a/tests/lib/utils.js b/tests/lib/utils.js index ac97bd33..ea12dde9 100644 --- a/tests/lib/utils.js +++ b/tests/lib/utils.js @@ -421,4 +421,54 @@ describe('utils', () => { }); }); }); + + describe('collectReportViolationAndSuggestionData', () => { + const CASES = [ + { + code: ` + context.report({ + node: {}, + message: "message1", + messageId: "messageId1", + data: { foo: 'hello' }, + fix(fixer) {}, + suggest: [{ + desc: "message2", + messageId: "messageId2", + data: { bar: 'world' }, + fix(fixer) {}, + }] + }); + `, + shouldMatch: [ + { + message: { type: 'Literal', value: 'message1' }, + messageId: { type: 'Literal', value: 'messageId1' }, + data: { type: 'ObjectExpression', properties: [{ key: { name: 'foo' } }] }, + fix: { type: 'FunctionExpression' }, + }, + { + message: { type: 'Literal', value: 'message2' }, + messageId: { type: 'Literal', value: 'messageId2' }, + data: { type: 'ObjectExpression', properties: [{ key: { name: 'bar' } }] }, + fix: { type: 'FunctionExpression' }, + }, + ], + }, + ]; + + it('behaves correctly', () => { + for (const testCase of CASES) { + const ast = espree.parse(testCase.code, { ecmaVersion: 6, range: true }); + const context = { getScope () {} }; // mock object + const reportNode = ast.body[0].expression; + const reportInfo = utils.getReportInfo(reportNode.arguments, context); + const data = utils.collectReportViolationAndSuggestionData(reportInfo); + assert( + lodash.isMatch(data, testCase.shouldMatch), + `Expected \n${inspect(data)}\nto match\n${inspect(testCase.shouldMatch)}` + ); + } + }); + }); });