-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
chore: disambiguate internal types LanguageOptions
and Rule
#19669
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'll leave it open for others to review
lib/linter/linter.js
Outdated
@@ -2645,7 +2644,7 @@ class Linter { | |||
/** | |||
* Defines a new linting rule. | |||
* @param {string} ruleId A unique rule identifier | |||
* @param {Rule} rule A rule object | |||
* @param {JSRuleDefinition} rule A rule object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no type JSRuleDefinition
defined or imported in this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e92fa18, thanks. I first thought to use JSRuleDefinition
in this PR but since this is a legacy method parameter and it's typed as Rule.RuleModule
in the TS types we can stick with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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
[ ] Add something to the core
[X] Other, please explain:
Fix internal types
What changes did you make? (Give an overview)
In the ESLint code, the types
LanguageOptions
andRule
are currently being used for both language-generic types (which now have equivalents in the core types) and for JavaScript-specific types. This PR resolves the ambiguities as follows:LanguageOptions
from core types is used.RuleDefinition
from core types is used.Linter.LanguageOptions
from the ESLint types is used with the aliasJSLanguageOptions
. This is probably the name we'll use when the JS language logic is moved to the@eslint/js
plugin (similar toCSSLanguageOptions
orMarkdownLanguageOptions
), so I chose to useJSLanguageOptions
to avoid confusion.Rule.RuleModule
from the ESLint types is used with the aliasRule
.Rule.RuleModule
is the type historically found in place ofJSRuleDefinition
in legacy code or for built-in rules.I also removed the unused
LanguageOptions
type fromlib/shared/types.js
.Is there anything you'd like reviewers to focus on?