DefinitelyTyped

DefinitelyTyped copied to clipboard

Please fill in this template.

• [X] Use a meaningful title for the pull request. Include the name of the package modified.
• [X] Test the change in your own code. (Compile and run.)
• [X] Add or edit tests to reflect the change.
• [X] Follow the advice from the readme.
• [X] Avoid common mistakes.
• [X] Run npm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

• [X] Provide a URL to documentation or source code which provides context for the suggested changes: https://eslint.org/docs/latest/user-guide/configuring/configuration-files#how-do-overrides-work "Override blocks can contain any configuration options that are valid in a regular config, ..."
• [ ] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

This change enables ESLint rules documentation on mouse hover for the rules in the overrides property.

Sep 21 '22 15:09 kazarmy

@kazarmy Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• eslint — on npm, on unpkg
  • owner-approval: @JounQin

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by a DT maintainer

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 62356,
  "author": "kazarmy",
  "headCommitOid": "614bb973554875f10ddf1cbcaadae2d283f54c37",
  "mergeBaseOid": "6738177c7d5eadd269d129467271f1138cfe9e4a",
  "lastPushDate": "2022-10-19T12:00:54.000Z",
  "lastActivityDate": "2022-10-20T10:44:41.000Z",
  "mergeOfferDate": "2022-10-19T16:33:49.000Z",
  "mergeRequestDate": "2022-10-20T10:44:41.000Z",
  "mergeRequestUser": "kazarmy",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "eslint",
      "kind": "edit",
      "files": [
        {
          "path": "types/eslint/eslint-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/eslint/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "pmdartus",
        "j-f1",
        "saadq",
        "JasonHK",
        "bradzacher",
        "JounQin"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "andrewbranch",
      "date": "2022-10-19T16:33:02.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "JounQin",
      "date": "2022-10-19T13:04:06.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1253906188,
  "ciResult": "pass"
}

Sep 21 '22 15:09 typescript-bot

🔔 @pmdartus @j-f1 @saadq @JasonHK @bradzacher @JounQin — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Sep 21 '22 15:09 typescript-bot

The overrides rules can be different from the base, right?

Sep 21 '22 23:09 JounQin

Yes, different rule sets can be enabled in an overrides entry -- they can even have different parsers. For example, for *.ts files, it would be @typescript-eslint rules with the @typescript-eslint/parser parser. However, I think it's better to have some typings available on the overrides property rather than none at all. At least mistakes with ESLint rule parameter names can be detected early.

Sep 22 '22 14:09 kazarmy

And from my testing, it does seem viable to have something like:

const config: Linter.Config<ESLintRules & MyTypeScriptESLintRules> = {
  // ...
};

if the rule typings need to be extended further.

Sep 22 '22 14:09 kazarmy

Ping @pmdartus @j-f1 @saadq @JasonHK @bradzacher @JounQin ... apparently @typescript-bot doesn't do reminders for this particular situation

Oct 10 '22 05:10 kazarmy

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @kazarmy.

(Ping @pmdartus, @j-f1, @saadq, @JasonHK, @bradzacher, @JounQin.)

Oct 10 '22 05:10 typescript-bot

And from my testing, it does seem viable to have something like:

const config: Linter.Config<ESLintRules, MyTypeScriptESLintRules> = {
  // ...
};

if the rule typings need to be extended further.

How about add another generic parameter?

Oct 10 '22 07:10 JounQin

Well there can be more than two sets of rule typings (e.g. for eslint, @typescript-eslint and jsonc) that need to be concatenated together so I think using & as seen here instead of adding generic parameters would be better.

Oct 10 '22 09:10 kazarmy

Anyway whether or not & or , is better, the config Rules typings should still be propagated to the overrides property so that some rules typings support is available even if not all.

Oct 10 '22 09:10 kazarmy

I would be really surprised if this change causes massive ecosystem effects.

Oct 13 '22 15:10 kazarmy

@JounQin Any other thoughts? This seems like an improvement to me.

Oct 17 '22 22:10 andrewbranch

@JounQin Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Oct 19 '22 12:10 typescript-bot

@kazarmy: Everything looks good here. I am ready to merge this PR (at 614bb97) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! :heart:

(@pmdartus, @j-f1, @saadq, @JasonHK, @bradzacher, @JounQin: you can do this too.)

Oct 19 '22 16:10 typescript-bot

Ready to merge

Oct 20 '22 10:10 kazarmy