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

feat: Pass --max-warnings value to formatters #16348

Merged
merged 3 commits into from Oct 7, 2022
Merged

feat: Pass --max-warnings value to formatters #16348

merged 3 commits into from Oct 7, 2022

Conversation

btmills
Copy link
Member

@btmills btmills commented Sep 25, 2022

Prerequisites checklist

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

[ ] 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
[ ] Other, please explain:

What changes did you make? (Give an overview)

This fixes #14881 by giving formatters access to the --max-warnings threshold when it is exceeded. The maxWarningsExceeded property matches what stylelint uses.

I had hoped to disable ESLint's own output message if the formatter provides its own message, but that would require even more API, so that's left as a future possibility if we get a request for it.

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

  1. The logic lives in the CLI's execute() method because that's where we're already doing the tooManyWarnings exit code calculation. We could instead pass the maxWarnings threshold through to ESLint/FlatESLint and have them replicate that calculation to create the maxWarningsExceeded object, but that seemed worse. I tested this logic as part of the CLI's formatter tests rather than the --max-warnings tests, though the logic is adjacent.
  2. Rather than adding a maxWarningsExceeded arg and piping that through, I put it in a resultsMeta object in case we want to add something else to it in the future.
  3. stylelint's maxWarningsExceeded property only exists if the threshold was exceeded. We could set it to null or false instead of omitting it, but I matched stylelint's behavior.
  4. I enabled this for both ESLint as well as FlatESLint because I only had to pass through the resultsMeta object. If we feel like we need more carrots to encourage switching to FlatESLint and that this would be a good one, I could remove it.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Sep 25, 2022
@netlify
Copy link

netlify bot commented Sep 25, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 5624b0b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/633fb579fbf32d000856fb97
😎 Deploy Preview https://deploy-preview-16348--docs-eslint.netlify.app/developer-guide/nodejs-api
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@btmills btmills added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features 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 Sep 25, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fasttime
Copy link
Contributor

fasttime commented Oct 1, 2022

I would suggest to update the definition of LoadedFormatter to include the new resultsMeta argument:

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

The definition of FormatterFunction could use an update with the maxWarningsExceeded property:

eslint/lib/shared/types.js

Lines 193 to 199 in 2dc338a

/**
* A formatter function.
* @callback FormatterFunction
* @param {LintResult[]} results The list of linting results.
* @param {{cwd: string, rulesMeta: Record<string, RuleMeta>}} [context] A context object.
* @returns {string | Promise<string>} Formatted text.
*/

@@ -132,13 +132,18 @@ Each `message` object contains information about the ESLint rule that was trigge
The formatter function receives an object as the second argument. The object has two properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

- The object has two properties:
+ The object has two or three properties:

or maybe just "the following properties"?

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 for catching this! Fixed in c772b6c.

Originally suggested by @fasttime in
#16348 (comment).

Internally, I define two types, `MaxWarningsExceeded` and `ResultsMeta`,
that the updated `LoadedFormatter` and `FormatterFunction` types can
use. I'm then able to reuse the `ResultsMeta` type instead of the
generic `Object` type in `ESLint` and `FlatESLint`'s `format` wrapper
functions.

Externally in the Node.js API docs, I describe the new second argument
to `LoadedFormatter`'s `format` method inline rather than separately
documenting the new types.

While working on this, I noticed that `FlatESLint` is using the
pre-PR #15727 `Formatter` type rather than `LoadedFormatter` and
`FormatterFunction`. I'll fix that in a follow-up PR to keep this PR
scoped to passing `maxWarningsExceeded` info.
@btmills
Copy link
Member Author

btmills commented Oct 7, 2022

I updated the typedefs in 5624b0b; see that commit message for more details, including why I'll be doing a follow-up PR to further improve formatter typedefs as used by FlatESLint.

@fasttime
Copy link
Contributor

fasttime commented Oct 7, 2022

Thanks @btmills! Nice work 👍🏻

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 173e820 into main Oct 7, 2022
20 checks passed
@mdjermanovic mdjermanovic deleted the issue14881 branch October 7, 2022 12:04
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 13, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.24.0` -> `8.25.0`](https://renovatebot.com/diffs/npm/eslint/8.24.0/8.25.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.25.0`](https://github.com/eslint/eslint/releases/tag/v8.25.0)

[Compare Source](eslint/eslint@v8.24.0...v8.25.0)

#### Features

-   [`173e820`](eslint/eslint@173e820) feat: Pass --max-warnings value to formatters ([#&#8203;16348](eslint/eslint#16348)) (Brandon Mills)
-   [`6964cb1`](eslint/eslint@6964cb1) feat: remove support for ignore files in FlatESLint ([#&#8203;16355](eslint/eslint#16355)) (Milos Djermanovic)
-   [`1cc4b3a`](eslint/eslint@1cc4b3a) feat: `id-length` counts graphemes instead of code units ([#&#8203;16321](eslint/eslint#16321)) (Sosuke Suzuki)

#### Documentation

-   [`90c6028`](eslint/eslint@90c6028) docs: Conflicting fixes ([#&#8203;16366](eslint/eslint#16366)) (Ben Perlmutter)
-   [`5a3fe70`](eslint/eslint@5a3fe70) docs: Add VS to integrations page ([#&#8203;16381](eslint/eslint#16381)) (Maria José Solano)
-   [`49bd1e5`](eslint/eslint@49bd1e5) docs: remove unused link definitions ([#&#8203;16376](eslint/eslint#16376)) (Nick Schonning)
-   [`3bd380d`](eslint/eslint@3bd380d) docs: typo cleanups for docs ([#&#8203;16374](eslint/eslint#16374)) (Nick Schonning)
-   [`b3a0837`](eslint/eslint@b3a0837) docs: remove duplicate words ([#&#8203;16378](eslint/eslint#16378)) (Nick Schonning)
-   [`a682562`](eslint/eslint@a682562) docs: add `BigInt` to `new-cap` docs ([#&#8203;16362](eslint/eslint#16362)) (Sosuke Suzuki)
-   [`f6d57fb`](eslint/eslint@f6d57fb) docs: Update docs README ([#&#8203;16352](eslint/eslint#16352)) (Ben Perlmutter)
-   [`7214347`](eslint/eslint@7214347) docs: fix logical-assignment-operators option typo ([#&#8203;16346](eslint/eslint#16346)) (Jonathan Wilsson)

#### Chores

-   [`1f78594`](eslint/eslint@1f78594) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).3.3 ([#&#8203;16397](eslint/eslint#16397)) (Milos Djermanovic)
-   [`8476a9b`](eslint/eslint@8476a9b) chore: Remove CODEOWNERS ([#&#8203;16375](eslint/eslint#16375)) (Nick Schonning)
-   [`720ff75`](eslint/eslint@720ff75) chore: use "ci" for Dependabot commit message ([#&#8203;16377](eslint/eslint#16377)) (Nick Schonning)
-   [`42f5479`](eslint/eslint@42f5479) chore: bump actions/stale from 5 to 6 ([#&#8203;16350](eslint/eslint#16350)) (dependabot\[bot])
-   [`e5e9e27`](eslint/eslint@e5e9e27) chore: remove `jsdoc` dev dependency ([#&#8203;16344](eslint/eslint#16344)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - 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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMjEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjIyNi4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1577
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --max-warning-count result object to the data argument in formatter function
3 participants