docs: disambiguate types `FormatterFunction` and `LoadedFormatter` #15727

fasttime · 2022-03-25T20:45:23Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[x] Other, please explain: type definitions changed, see below.

What changes did you make? (Give an overview)

Add type FormatterFunction
Rename type Formatter to LoadedFormatter
Use FormatterFunction in the return type of CLIEngine.getFormatter
Move type LintResult to shared types because it is referenced by FormatterFunction
Fix a misspelling of LintResult in JSDoc
docs: Update/clarify some text about custom formatters
~~docs: Rename variable formatter to loadedFormatter in code examples~~

Is there anything you'd like reviewers to focus on?

I don't think there is a way to test type definitions or any other of the changes listed above at this time, so probably everything needs a careful human review.

eslint-github-bot · 2022-03-25T20:45:28Z

Hi @fasttime!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

Fixes #15654

mdjermanovic · 2022-03-28T10:50:50Z

lib/cli-engine/cli-engine.js

@@ -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 */


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?

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.

mdjermanovic · 2022-03-28T11:13:49Z

lib/shared/types.js

+ * 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.


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.

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.

nzakas · 2022-03-30T00:32:22Z

docs/developer-guide/nodejs-api.md

@@ -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");


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

* Remove redundant `LintResult` type import * Remove note about `context` argument of `FormatterFunction` from JSDoc * docs: Revert to using variable name `formatter` in code examples

fasttime · 2022-04-01T14:11:16Z

@mdjermanovic, @nzakas Thanks for your feedback. I've done some changes, as per discussion:

Remove redundant LintResult type import
Remove note about context argument of FormatterFunction from JSDoc
docs: Revert to using variable name formatter in code examples

nzakas

LGTM. Thanks!

Leaving open for @mdjermanovic to verify his feedback was addressed

mdjermanovic

LGTM, thanks for contributing!

…15727) * chore: disambiguate types `FormatterFunction` and `LoadedFormatter` Fixes #15654 * Code review update * Remove redundant `LintResult` type import * Remove note about `context` argument of `FormatterFunction` from JSDoc * docs: Revert to using variable name `formatter` in code examples

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.12.0` -> `8.13.0`](https://renovatebot.com/diffs/npm/eslint/8.12.0/8.13.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.13.0`](https://github.com/eslint/eslint/releases/v8.13.0) [Compare Source](eslint/eslint@v8.12.0...v8.13.0) #### Features - [`274acbd`](eslint/eslint@274acbd) feat: fix no-eval logic for `this` in arrow functions ([#15755](eslint/eslint#15755)) (Milos Djermanovic) #### Bug Fixes - [`97b57ae`](eslint/eslint@97b57ae) fix: invalid operator in operator-assignment messages ([#15759](eslint/eslint#15759)) (Milos Djermanovic) #### Documentation - [`c32482e`](eslint/eslint@c32482e) docs: Typo in space-infix-ops docs ([#15754](eslint/eslint#15754)) (kmin-jeong) - [`f2c2d35`](eslint/eslint@f2c2d35) docs: disambiguate types `FormatterFunction` and `LoadedFormatter` ([#15727](eslint/eslint#15727)) (Francesco Trotta) #### Chores - [`bb4c0d5`](eslint/eslint@bb4c0d5) chore: Refactor docs to work with docs.eslint.org ([#15744](eslint/eslint#15744)) (Nicholas C. Zakas) - [`d36f12f`](eslint/eslint@d36f12f) chore: remove `lib/init` from eslint config ([#15748](eslint/eslint#15748)) (Milos Djermanovic) - [`a59a4e6`](eslint/eslint@a59a4e6) chore: replace `trimLeft`/`trimRight` with `trimStart`/`trimEnd` ([#15750](eslint/eslint#15750)) (Milos Djermanovic) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1296 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

…slint#15727) * chore: disambiguate types `FormatterFunction` and `LoadedFormatter` Fixes eslint#15654 * Code review update * Remove redundant `LintResult` type import * Remove note about `context` argument of `FormatterFunction` from JSDoc * docs: Revert to using variable name `formatter` in code examples

…tter` (eslint#15727)" This reverts commit 0142501.

eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 25, 2022

chore: disambiguate types FormatterFunction and LoadedFormatter

33fc3af

Fixes #15654

eslint-github-bot bot added the chore This change is not user-facing label Mar 25, 2022

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 28, 2022

mdjermanovic reviewed Mar 28, 2022

View reviewed changes

nzakas reviewed Mar 30, 2022

View reviewed changes

Code review update

1c0ad3d

* Remove redundant `LintResult` type import * Remove note about `context` argument of `FormatterFunction` from JSDoc * docs: Revert to using variable name `formatter` in code examples

nzakas approved these changes Apr 2, 2022

View reviewed changes

mdjermanovic changed the title ~~chore: disambiguate types FormatterFunction and LoadedFormatter~~ docs: disambiguate types FormatterFunction and LoadedFormatter Apr 3, 2022

eslint-github-bot bot added the documentation Relates to ESLint's documentation label Apr 3, 2022

mdjermanovic approved these changes Apr 4, 2022

View reviewed changes

mdjermanovic merged commit f2c2d35 into eslint:main Apr 4, 2022

sync-by-unito bot mentioned this pull request May 1, 2022

build(deps-dev): bump eslint from 7.32.0 to 8.14.0 numbersprotocol/capture-lite#1569

Closed

srijan-deepsource added a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022

Revert "docs: disambiguate types FormatterFunction and `LoadedForma…

2d83440

…tter` (eslint#15727)" This reverts commit 0142501.

eslint-github-bot bot locked and limited conversation to collaborators Oct 2, 2022

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: disambiguate types `FormatterFunction` and `LoadedFormatter` #15727

docs: disambiguate types `FormatterFunction` and `LoadedFormatter` #15727

fasttime commented Mar 25, 2022 •

edited

eslint-github-bot bot commented Mar 25, 2022

mdjermanovic Mar 28, 2022

fasttime Mar 28, 2022

mdjermanovic Mar 28, 2022

fasttime Mar 28, 2022 •

edited

nzakas Mar 30, 2022

fasttime commented Apr 1, 2022

nzakas left a comment

mdjermanovic left a comment

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

	* @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.

docs: disambiguate types FormatterFunction and LoadedFormatter #15727

docs: disambiguate types FormatterFunction and LoadedFormatter #15727

Conversation

fasttime commented Mar 25, 2022 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

eslint-github-bot bot commented Mar 25, 2022

mdjermanovic Mar 28, 2022

Choose a reason for hiding this comment

fasttime Mar 28, 2022

Choose a reason for hiding this comment

mdjermanovic Mar 28, 2022

Choose a reason for hiding this comment

fasttime Mar 28, 2022 • edited

Choose a reason for hiding this comment

nzakas Mar 30, 2022

Choose a reason for hiding this comment

fasttime commented Apr 1, 2022

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

docs: disambiguate types `FormatterFunction` and `LoadedFormatter` #15727

docs: disambiguate types `FormatterFunction` and `LoadedFormatter` #15727

fasttime commented Mar 25, 2022 •

edited

fasttime Mar 28, 2022 •

edited