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.

Sign up for GitHub

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

Jump to bottom

docs: disambiguate types FormatterFunction and LoadedFormatter #15727

Merged
merged 2 commits into from Apr 4, 2022
Merged

docs: disambiguate types FormatterFunction and LoadedFormatter #15727

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33fc3af
chore: disambiguate types `FormatterFunction` and `LoadedFormatter`
fasttime Mar 25, 2022
1c0ad3d
Code review update
fasttime Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 11 additions & 11 deletions docs/developer-guide/nodejs-api.md
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ While ESLint is designed to be run on the command line, it's possible to use ESL
* [LintMessage type][lintmessage]
* [SuppressedLintMessage type][suppressedlintmessage]
* [EditInfo type][editinfo]
* [Formatter type][formatter]
* [LoadedFormatter type][loadedformatter]
* [SourceCode](#sourcecode)
* [splitLines()](#sourcecodesplitlines)
* [Linter](#linter)
Expand Down Expand Up @@ -56,8 +56,8 @@ const { ESLint } = require("eslint");
const results = await eslint.lintFiles(["lib/**/*.js"]);

// 3. Format the results.
const formatter = await eslint.loadFormatter("stylish");
const resultText = formatter.format(results);
const loadedFormatter = await eslint.loadFormatter("stylish");
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure this change makes sense. The variable name doesn’t need to match the type name.

const resultText = loadedFormatter.format(results);

// 4. Output it.
console.log(resultText);
Expand All @@ -83,8 +83,8 @@ const { ESLint } = require("eslint");
await ESLint.outputFixes(results);

// 4. Format the results.
const formatter = await eslint.loadFormatter("stylish");
const resultText = formatter.format(results);
const loadedFormatter = await eslint.loadFormatter("stylish");
const resultText = loadedFormatter.format(results);

// 5. Output it.
console.log(resultText);
Expand Down Expand Up @@ -268,7 +268,7 @@ This method checks if a given file is ignored by your configuration.
### ◆ eslint.loadFormatter(nameOrPath)

```js
const formatter = await eslint.loadFormatter(nameOrPath);
const loadedFormatter = await eslint.loadFormatter(nameOrPath);
```

This method loads a formatter. Formatters convert lint results to a human- or machine-readable string.
Expand All @@ -287,8 +287,8 @@ This method loads a formatter. Formatters convert lint results to a human- or ma

#### Return Value

* (`Promise<Formatter>`)<br>
The promise that will be fulfilled with a [Formatter] object.
* (`Promise<LoadedFormatter>`)<br>
The promise that will be fulfilled with a [LoadedFormatter] object.

### ◆ ESLint.version

Expand Down Expand Up @@ -430,9 +430,9 @@ The `EditInfo` value is information to edit text. The `fix` and `suggestions` pr

This edit information means replacing the range of the `range` property by the `text` property value. It's like `sourceCodeText.slice(0, edit.range[0]) + edit.text + sourceCodeText.slice(edit.range[1])`. Therefore, it's an add if the `range[0]` and `range[1]` property values are the same value, and it's removal if the `text` property value is empty string.

### ◆ Formatter type
### ◆ LoadedFormatter type

The `Formatter` value is the object to convert the [LintResult] objects to text. The [eslint.loadFormatter()][eslint-loadformatter] method returns it. It has the following method:
The `LoadedFormatter` value is the object to convert the [LintResult] objects to text. The [eslint.loadFormatter()][eslint-loadformatter] method returns it. It has the following method:

* `format` (`(results: LintResult[]) => string | Promise<string>`)<br>
The method to convert the [LintResult] objects to text.
Expand Down Expand Up @@ -960,5 +960,5 @@ ruleTester.run("my-rule", myRule, {
[lintmessage]: #-lintmessage-type
[suppressedlintmessage]: #-suppressedlintmessage-type
[editinfo]: #-editinfo-type
[formatter]: #-formatter-type
[loadedformatter]: #-loadedformatter-type
[linter]: #linter
6 changes: 3 additions & 3 deletions docs/developer-guide/working-with-custom-formatters.md
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@

While ESLint has some built-in formatters available to format the linting results, it's also possible to create and distribute your own custom formatters. You can include custom formatters in your project directly or create an npm package to distribute them separately.

Each formatter is just a function that receives a `results` object and returns a string. For example, the following is how the `json` built-in formatter is implemented:
Each formatter is just a function that receives a `results` object and a `context` and returns a string. For example, the following is how the `json` built-in formatter is implemented:

```js
//my-awesome-formatter.js
Expand All @@ -11,7 +11,7 @@ module.exports = function(results, context) {
};
```

Formatter can also be an async function (from ESLint v8.4.0), the following shows a simple example:
A formatter can also be an async function (from ESLint v8.4.0), the following shows a simple example:

```js
//my-awesome-formatter.js
Expand Down Expand Up @@ -280,7 +280,7 @@ warning no-unused-vars (https://eslint.org/docs/rules/no-unused-vars)

## Passing Arguments to Formatters

While custom formatter do not receive arguments in addition to the results object, it is possible to pass additional data into formatters.
While formatter functions do not receive arguments in addition to the results object and the context, it is possible to pass additional data into custom formatters using the methods described below.

## Using Environment Variables

Expand Down
4 changes: 3 additions & 1 deletion lib/cli-engine/cli-engine.js
View file
Open in desktop
Expand Up @@ -56,6 +56,8 @@ const validFixTypes = new Set(["directive", "problem", "suggestion", "layout"]);
/** @typedef {import("../shared/types").Plugin} Plugin */
/** @typedef {import("../shared/types").RuleConf} RuleConf */
/** @typedef {import("../shared/types").Rule} Rule */
/** @typedef {import("../shared/types").LintResult} LintResult */
Copy link
Member

Choose a reason for hiding this comment

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

In this file, there is already a definition for LintResult (starting from line 92), and it's different than this one, so it seems we shouldn't import the definition from shared/types here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @mdjermanovic, I missed that one! The definition of LintResult in cli-engine.js is identical to the shared one, except that it does not include usedDeprecatedRules. This makes sense somehow, since that property is attached later outside the CLIEngine:

eslint/lib/eslint/eslint.js

Lines 408 to 410 in 5d60812

for (const result of results) {
Object.defineProperty(result, "usedDeprecatedRules", descriptor);
}

The other definition could be better modeled with type inheritance, but changing it would be outside the scope of this PR, I think. I will remove the import if nobody comes up with a different suggestion.

/** @typedef {import("../shared/types").FormatterFunction} FormatterFunction */
/** @typedef {ReturnType<CascadingConfigArrayFactory.getConfigArrayForFile>} ConfigArray */
/** @typedef {ReturnType<ConfigArray.extractConfig>} ExtractedConfig */

Expand Down Expand Up @@ -1002,7 +1004,7 @@ class CLIEngine {
* @param {string} [format] The name of the format to load or the path to a
* custom formatter.
* @throws {any} As may be thrown by requiring of formatter
* @returns {(Function|null)} The formatter function or null if the `format` is not a string.
* @returns {(FormatterFunction|null)} The formatter function or null if the `format` is not a string.
*/
getFormatter(format) {

Expand Down
23 changes: 4 additions & 19 deletions lib/eslint/eslint.js
View file
Open in desktop
Expand Up @@ -35,10 +35,11 @@ const { version } = require("../../package.json");
/** @typedef {import("../shared/types").SuppressedLintMessage} SuppressedLintMessage */
/** @typedef {import("../shared/types").Plugin} Plugin */
/** @typedef {import("../shared/types").Rule} Rule */
/** @typedef {import("../shared/types").LintResult} LintResult */

/**
* The main formatter object.
* @typedef Formatter
* @typedef LoadedFormatter
* @property {function(LintResult[]): string | Promise<string>} format format function.
*/

Expand Down Expand Up @@ -74,22 +75,6 @@ const { version } = require("../../package.json");
* @property {Object} definition The plugin definition.
*/

/**
* A linting result.
* @typedef {Object} LintResult
* @property {string} filePath The path to the file that was linted.
* @property {LintMessage[]} messages All of the messages for the result.
* @property {SuppressedLintMessage[]} suppressedMessages All of the suppressed messages for the result.
* @property {number} errorCount Number of errors for the result.
* @property {number} fatalErrorCount Number of fatal errors for the result.
* @property {number} warningCount Number of warnings for the result.
* @property {number} fixableErrorCount Number of fixable errors for the result.
* @property {number} fixableWarningCount Number of fixable warnings for the result.
* @property {string} [source] The source code of the file that was linted.
* @property {string} [output] The source code of the file that was linted, with as many fixes applied as possible.
* @property {DeprecatedRuleInfo[]} usedDeprecatedRules The list of used deprecated rules.
*/

/**
* Private members for the `ESLint` instance.
* @typedef {Object} ESLintPrivateMembers
Expand Down Expand Up @@ -619,7 +604,7 @@ class ESLint {
* - `@foo` → `@foo/eslint-formatter`
* - `@foo/bar` → `@foo/eslint-formatter-bar`
* - A file path ... Load the file.
* @returns {Promise<Formatter>} A promise resolving to the formatter object.
* @returns {Promise<LoadedFormatter>} A promise resolving to the formatter object.
* This promise will be rejected if the given formatter was not found or not
* a function.
*/
Expand All @@ -639,7 +624,7 @@ class ESLint {

/**
* The main formatter method.
* @param {LintResults[]} results The lint results to format.
* @param {LintResult[]} results The lint results to format.
* @returns {string | Promise<string>} The formatted lint results.
*/
format(results) {
Expand Down
24 changes: 24 additions & 0 deletions lib/shared/types.js
View file
Open in desktop
Expand Up @@ -173,3 +173,27 @@ module.exports = {};
* @property {string} ruleId The rule ID.
* @property {string[]} replacedBy The rule IDs that replace this deprecated rule.
*/

/**
* A linting result.
* @typedef {Object} LintResult
* @property {string} filePath The path to the file that was linted.
* @property {LintMessage[]} messages All of the messages for the result.
* @property {SuppressedLintMessage[]} suppressedMessages All of the suppressed messages for the result.
* @property {number} errorCount Number of errors for the result.
* @property {number} fatalErrorCount Number of fatal errors for the result.
* @property {number} warningCount Number of warnings for the result.
* @property {number} fixableErrorCount Number of fixable errors for the result.
* @property {number} fixableWarningCount Number of fixable warnings for the result.
* @property {string} [source] The source code of the file that was linted.
* @property {string} [output] The source code of the file that was linted, with as many fixes applied as possible.
* @property {DeprecatedRuleInfo[]} usedDeprecatedRules The list of used deprecated rules.
*/

/**
* A formatter function.
* @callback FormatterFunction
* @param {LintResult[]} results The list of linting results.
* @param {{cwd: string, rulesMeta: Record<string, RuleMeta>}} [context] A context object. If the formatter is used with the legacy API, this argument may be unspecified or have a different value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {{cwd: string, rulesMeta: Record<string, RuleMeta>}} [context] A context object. If the formatter is used with the legacy API, this argument may be unspecified or have a different value.
* @param {{cwd: string, rulesMeta: Record<string, RuleMeta>}} [context] A context object.

I think a note about different APIs isn't necessary, as it could be added to basically any type we have. For example, LintResult can have more or fewer properties depending on ESLint version.

Copy link
Member Author

@fasttime fasttime Mar 28, 2022 •
edited

Choose a reason for hiding this comment

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

Okay, I included that note because it is mentioned in the developer guide:

eslint/docs/developer-guide/working-with-custom-formatters.md

Line 151 in 51c37b1

**Note:** if a linting is executed by deprecated `CLIEngine` class, the `context` argument may be a different value because it is up to the API users. Please check whether the `context` argument is an expected value or not if you want to support legacy environments.

So the legacy environments are not those using old ESLint versions, but those that use the deprecated CLIEngine class. Granted, my wording doesn't make it clear at all. But anyway, it's fine to remove that note.

* @returns {string | Promise<string>} Formatted text.
*/