Skip to content

Commit

Permalink
New: Add context.report({ messageId }) (fixes #6740) (#9165)
Browse files Browse the repository at this point in the history 
* New: Add context.report({ messageId }) (fixes #6740)

* Chore: Extract out loop variables in rule-tester.js

* New: RuleTester checks the messageId of expected errors

* Chore: Extract the {{ }} substitutions into a separate file

* Chore: Test interpolate()

* Docs: document messageIds

* Fix: Node 4 compatibility

* Docs: Specify `node` in messageIds example

* Update: Throw if a rule uses a messageId, but doesn’t have meta.messages

* Fix: Don’t mutate the descriptor

* Chore: Use hOP instead of Object.keys().indexOf

* Docs: Add explanation for messageIds

* Chore: Fix lint errors

* Docs: Show how to use placeholders in messages
  • Loading branch information
@j-f1 @kaicataldo
j-f1 authored and kaicataldo committed Jan 7, 2018
1 parent fc7f404 commit 6ab04b5
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 41 deletions.
57 changes: 57 additions & 0 deletions docs/developer-guide/working-with-rules.md
View file
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion lib/linter.js
View file
Expand Up @@ -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(
Expand All @@ -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);

Expand Down
47 changes: 34 additions & 13 deletions lib/report-translator.js
View file
Expand Up @@ -11,6 +11,7 @@

const assert = require("assert");
const ruleFixer = require("./util/rule-fixer");
const interpolate = require("./util/interpolate");

//------------------------------------------------------------------------------
// Typedefs
Expand Down Expand Up @@ -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].
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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;
Expand All @@ -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),
Expand All @@ -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
Expand Down
67 changes: 41 additions & 26 deletions lib/testers/rule-tester.js
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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.");
}
}
}
Expand Down
24 changes: 24 additions & 0 deletions lib/util/interpolate.js
View file
@@ -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;
});
};
57 changes: 56 additions & 1 deletion tests/lib/report-translator.js
View file
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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.", () => {
Expand Down

0 comments on commit 6ab04b5

Please sign in to comment.