Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: Add suggestions API #12384

Merged
merged 13 commits into from Nov 22, 2019
Merged
47 changes: 45 additions & 2 deletions docs/developer-guide/nodejs-api.md
Expand Up @@ -108,7 +108,7 @@ The most important method on `Linter` is `verify()`, which initiates linting of
* `preprocess` - (optional) A function that [Processors in Plugins](/docs/developer-guide/working-with-plugins.md#processors-in-plugins) documentation describes as the `preprocess` method.
* `postprocess` - (optional) A function that [Processors in Plugins](/docs/developer-guide/working-with-plugins.md#processors-in-plugins) documentation describes as the `postprocess` method.
* `filterCodeBlock` - (optional) A function that decides which code blocks the linter should adopt. The function receives two arguments. The first argument is the virtual filename of a code block. The second argument is the text of the code block. If the function returned `true` then the linter adopts the code block. If the function was omitted, the linter adopts only `*.js` code blocks. If you provided a `filterCodeBlock` function, it overrides this default behavior, so the linter doesn't adopt `*.js` code blocks automatically.
* `disableFixes` - (optional) when set to `true`, the linter doesn't make the `fix` property of the lint result.
* `disableFixes` - (optional) when set to `true`, the linter doesn't make either the `fix` or `suggestions` property of the lint result.
* `allowInlineConfig` - (optional) set to `false` to disable inline comments from changing ESLint rules.
* `reportUnusedDisableDirectives` - (optional) when set to `true`, adds reported errors for unused `eslint-disable` directives when no problems would be reported in the disabled area anyway.

Expand Down Expand Up @@ -170,6 +170,7 @@ The information available for each linting message is:
* `endColumn` - the end column of the range on which the error occurred (this property is omitted if it's not range).
* `endLine` - the end line of the range on which the error occurred (this property is omitted if it's not range).
* `fix` - an object describing the fix for the problem (this property is omitted if no fix is available).
* `suggestions` - an array of objects describing possible lint fixes for editors to programmatically enable (see details in the [Working with Rules docs](./working-with-rules.md#providing-suggestions)).

Linting message objects have a deprecated `source` property. This property **will be removed** from linting messages in an upcoming breaking release. If you depend on this property, you should now use the `SourceCode` instance provided by the linter.

Expand Down Expand Up @@ -437,9 +438,23 @@ The return value is an object containing the results of the linting operation. H
column: 13,
nodeType: "ExpressionStatement",
fix: { range: [12, 12], text: ";" }
}, {
ruleId: "no-useless-escape",
severity: 1,
message: "disallow unnecessary escape characters",
line: 1,
column: 10,
nodeType: "ExpressionStatement",
suggestions: [{
desc: "Remove unnecessary escape. This maintains the current functionality.",
fix: { range: [9, 10], text: "" }
}, {
desc: "Escape backslash to include it in the RegExp.",
fix: { range: [9, 9], text: "\\" }
}]
}],
errorCount: 1,
warningCount: 0,
warningCount: 1,
fixableErrorCount: 1,
fixableWarningCount: 0,
source: "\"use strict\"\n"
Expand Down Expand Up @@ -865,6 +880,7 @@ In addition to the properties above, invalid test cases can also have the follow
* `column` (number): The 1-based column number of the reported location
* `endLine` (number): The 1-based line number of the end of the reported location
* `endColumn` (number): The 1-based column number of the end of the reported location
* `suggestions` (array): An array of objects with suggestion details to check. See [Testing Suggestions](#testing-suggestions) for details

If a string is provided as an error instead of an object, the string is used to assert the `message` of the error.
* `output` (string, optional): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null`, asserts that none of the reported problems suggest autofixes.
Expand All @@ -880,6 +896,33 @@ Any additional properties of a test case will be passed directly to the linter a

If a valid test case only uses the `code` property, it can optionally be provided as a string containing the code, rather than an object with a `code` key.

#### Testing Suggestions

Suggestions can be tested by defining a `suggestions` key on an errors object. The options to check for the suggestions are the following (all are optional):
* `desc` (string): The suggestion `desc` value
* `messageId` (string): The suggestion `messageId` value for suggestions that use `messageId`s
* `fix` (object): An object matching the suggestion `fix` object output with fields `text` and `range`
* `output` (string): A code string representing the result of applying the suggestion fix to the input code

Example:

```js
ruleTester.run("my-rule-for-no-foo", rule, {
valid: [],
invalid: [{
code: "var foo;",
errors: [{
suggestions: [{
desc: "Rename identifier 'foo' to 'bar'",
messageId: "renameFoo",
output: "var bar;",
fix: { text: "bar", range: [4, 7] }
}]
}]
}]
})
```

### Customizing RuleTester

`RuleTester` depends on two functions to run tests: `describe` and `it`. These functions can come from various places:
Expand Down
4 changes: 2 additions & 2 deletions docs/developer-guide/unit-tests.md
Expand Up @@ -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.
74 changes: 74 additions & 0 deletions docs/developer-guide/working-with-rules.md
Expand Up @@ -62,6 +62,7 @@ The source file for a rule exports an object with the following properties.
* `category` (string) specifies the heading under which the rule is listed in the [rules index](../rules/)
* `recommended` (boolean) is whether the `"extends": "eslint:recommended"` property in a [configuration file](../user-guide/configuring.md#extending-configuration-files) enables the rule
* `url` (string) specifies the URL at which the full documentation can be accessed
* `suggestion` (boolean) specifies whether rules can return suggestions (defaults to false if omitted)

In a custom rule or plugin, you can omit `docs` or include any properties that you need in it.

Expand Down Expand Up @@ -344,6 +345,79 @@ Best practices for fixes:

* This fixer can just select a quote type arbitrarily. If it guesses wrong, the resulting code will be automatically reported and fixed by the [`quotes`](/docs/rules/quotes.md) rule.

### Providing Suggestions

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 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: "Unnecessary escape character: \\{{character}}.",
data: { character },
suggest: [
{
desc: "Remove the `\\`. This maintains the current functionality.",
fix: function(fixer) {
return fixer.removeRange(range);
}
},
{
desc: "Replace the `\\` with `\\\\` to include the actual backslash character.",
fix: function(fixer) {
return fixer.insertTextBeforeRange(range, "\\");
}
}
]
});
```

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 conform 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.

Best practices for suggestions:

1. Don't try to do too much and suggest large refactors that could introduce a lot of breaking changes.
1. As noted above, don't try to conform to user-defined styles.

wdoug marked this conversation as resolved.
Show resolved Hide resolved
#### 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: {
unnecessaryEscape: "Unnecessary escape character: \\{{character}}.",
removeEscape: "Remove the `\\`. This maintains the current functionality.",
escapeBackslash: "Replace the `\\` with `\\\\` to include the actual backslash character."
}
},
create: function(context) {
// ...
context.report({
node: node,
messageId: 'unnecessaryEscape',
data: { character },
suggest: [
{
messageId: "removeEscape",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented suggestions support for messageIds. Do we want to do it this way, or should it be a different key and different format in the meta info? For example, I could easily change this to be descriptionId and put the associated descriptions in meta.suggestionDescriptions or something instead of overloading meta.messages and messageIds. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question!

I don't have a problem with reusing meta.messages itself, but I would be curious to know if other team members think it could be confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see meta.messages as general-purpose bucket for i18n. So I think it should be fine to just reuse it.

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:
Expand Down
80 changes: 73 additions & 7 deletions lib/linter/report-translator.js
Expand Up @@ -26,6 +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, messageId?: string, fix: Function}>} suggest Suggestion descriptions and functions to create a the associated fixes.
*/

/**
Expand All @@ -34,14 +35,15 @@ const interpolate = require("./interpolate");
* @property {string} ruleId
* @property {(0|1|2)} severity
* @property {(string|undefined)} message
* @property {(string|undefined)} messageId
* @property {(string|undefined)} [messageId]
* @property {number} line
* @property {number} column
* @property {(number|undefined)} endLine
* @property {(number|undefined)} endColumn
* @property {(number|undefined)} [endLine]
* @property {(number|undefined)} [endColumn]
* @property {(string|null)} nodeType
* @property {string} source
* @property {({text: string, range: (number[]|null)}|null)} fix
* @property {({text: string, range: (number[]|null)}|null)} [fix]
* @property {Array<{text: string, range: (number[]|null)}|null>} [suggestions]
*/

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -182,6 +184,29 @@ function normalizeFixes(descriptor, sourceCode) {
return fix;
}

/**
* 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.
* @param {Object} messages Object of meta messages for the rule.
* @returns {Array<SuggestionResult>} The suggestions for the descriptor
*/
function mapSuggestions(descriptor, sourceCode, messages) {
if (!descriptor.suggest || !Array.isArray(descriptor.suggest)) {
return [];
}

return descriptor.suggest.map(suggestInfo => {
const computedDesc = suggestInfo.desc || messages[suggestInfo.messageId];

return {
...suggestInfo,
desc: interpolate(computedDesc, suggestInfo.data),
fix: normalizeFixes(suggestInfo, sourceCode)
};
});
}

/**
* Creates information about the report from a descriptor
* @param {Object} options Information about the problem
Expand All @@ -192,6 +217,7 @@ function normalizeFixes(descriptor, sourceCode) {
* @param {string} [options.messageId] The error message ID.
* @param {{start: SourceLocation, end: (SourceLocation|null)}} options.loc Start and end location
* @param {{text: string, range: (number[]|null)}} options.fix The fix object
* @param {Array<{text: string, range: (number[]|null)}>} options.suggestions The array of suggestions objects
* @returns {function(...args): ReportInfo} Function that returns information about the report
*/
function createProblem(options) {
Expand Down Expand Up @@ -221,9 +247,47 @@ function createProblem(options) {
problem.fix = options.fix;
}

if (options.suggestions && options.suggestions.length > 0) {
problem.suggestions = options.suggestions;
}

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.
Expand All @@ -242,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.");
Expand All @@ -267,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,
Expand All @@ -275,7 +340,8 @@ module.exports = function createReportTranslator(metadata) {
message: interpolate(computedMessage, descriptor.data),
messageId: descriptor.messageId,
loc: normalizeReportLoc(descriptor),
fix: metadata.disableFixes ? null : normalizeFixes(descriptor, metadata.sourceCode)
fix: metadata.disableFixes ? null : normalizeFixes(descriptor, metadata.sourceCode),
suggestions: metadata.disableFixes ? [] : mapSuggestions(descriptor, metadata.sourceCode, messages)
});
};
};