-
-
Notifications
You must be signed in to change notification settings - Fork 24
feat: Support new DeprecatedInfo format for rule meta.deprecated
#730
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
Changes from 5 commits
8c38608
4436953
a0f8b6f
783837b
22be021
d90c3bb
479b0d8
ede4a08
4903b0d
c17306f
9cf3d58
fc190a1
36227dd
3a2a6b3
f8c7b45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { | |||||
| import { findConfigEmoji, getConfigsForRule } from './plugin-configs.js'; | ||||||
| import { | ||||||
| RuleModule, | ||||||
| DeprecatedInfo, | ||||||
| Plugin, | ||||||
| ConfigsToRules, | ||||||
| ConfigEmojis, | ||||||
|
|
@@ -104,6 +105,7 @@ const RULE_NOTICES: { | |||||
| fixable: boolean; | ||||||
| hasSuggestions: boolean; | ||||||
| urlConfigs?: string; | ||||||
| deprecatedInfo: boolean | DeprecatedInfo | undefined; | ||||||
| replacedBy: readonly string[] | undefined; | ||||||
| plugin: Plugin; | ||||||
| pluginPrefix: string; | ||||||
|
|
@@ -187,8 +189,8 @@ const RULE_NOTICES: { | |||||
| return `${emojis.join('')} ${sentences}`; | ||||||
| }, | ||||||
|
|
||||||
| // Deprecated notice has optional "replaced by" rules list. | ||||||
| [NOTICE_TYPE.DEPRECATED]: ({ | ||||||
| deprecatedInfo, | ||||||
| replacedBy, | ||||||
| plugin, | ||||||
| pluginPrefix, | ||||||
|
|
@@ -197,6 +199,43 @@ const RULE_NOTICES: { | |||||
| ruleName, | ||||||
| urlRuleDoc, | ||||||
| }) => { | ||||||
| // use object type `DeprecatedInfo` | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to include |
||||||
| if (typeof deprecatedInfo === 'object') { | ||||||
| const replacementRuleList = (deprecatedInfo.replacedBy ?? []) | ||||||
| .map(({ rule }) => | ||||||
| rule && rule.name | ||||||
| ? rule.url | ||||||
| ? `[\`${rule.name}\`](${rule.url})` | ||||||
| : `\`${rule.name}\`` | ||||||
| : undefined, | ||||||
| ) | ||||||
| .filter((rule): rule is string => typeof rule === 'string'); | ||||||
|
|
||||||
| return `${EMOJI_DEPRECATED} This rule is deprecated${ | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, sorry, I'm not a native speaker of english |
||||||
| deprecatedInfo.deprecatedSince | ||||||
| ? ` since v${deprecatedInfo.deprecatedSince}.` | ||||||
| : '.' | ||||||
| }${ | ||||||
| replacementRuleList.length > 0 | ||||||
| ? ` It was replaced by ${String(replacementRuleList)}.` | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want a comma-separative list with spaces.
Suggested change
I'm fixing in the existing code too: #731
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be improved even further, like ? ` It was replaced by ${
replacementRuleList.length > 1
? `${replacementsRuleLists.slice(0, -1).join(', ')} and ${String(replacementsRuleList.at(-1))}`
: replacementsRuleLists[0]
}.`
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to that. Would want to extract a helper function for it. |
||||||
| : '' | ||||||
| }${ | ||||||
| // use DeprecatedInfo#url to inform about the reasons | ||||||
| deprecatedInfo.url | ||||||
| ? `${EOL}${EOL}Read more at [${new URL(deprecatedInfo.url).hostname}](${deprecatedInfo.url})` | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an interesting idea to use the hostname, but I think I'd rather just use this, and keep everything related to deprecations on the same line.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See this comment for desired format: #730 (comment) |
||||||
| : '' | ||||||
| }`; | ||||||
| } | ||||||
|
|
||||||
| // warn and use fallback | ||||||
| console.warn( | ||||||
| [ | ||||||
| 'The two top-level properties `deprecated` and `replacedBy` are deprecated since eslint 9.21.0.', | ||||||
|
bmish marked this conversation as resolved.
Outdated
|
||||||
| 'Please consider using the new object type `DeprecatedInfo`.', | ||||||
| 'https://eslint.org/docs/latest/extend/rule-deprecation#-deprecatedinfo-type', | ||||||
| ].join('\n'), | ||||||
| ); | ||||||
|
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, while it's a nice idea to warn about this, I'd rather use a new lint rule in eslint-plugin-eslint-plugin to do this: eslint-doc-generator is not really intended to warn about deprecations or older styles. That's the job of eslint-plugin-eslint-plugin. So I'd like to remove the warning. |
||||||
| const replacementRuleList = (replacedBy ?? []).map((replacementRuleName) => | ||||||
| getLinkToRule( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To your question about this, I think I would still like to automatically fill in links for rules where we can. Including URLs in rule definitions is burdensome, and while rules theoretically could be responsible for it now, I assume many will omit it. However, we can try to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to give it a shot. But first let me check if I understood it correctly and considered the relevant cases: In case a user of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not totally sure. If any scenarios are ambiguous or overly-complicated, we can just omit the link in them and consider as a follow-up. |
||||||
| replacementRuleName, | ||||||
|
|
@@ -210,6 +249,7 @@ const RULE_NOTICES: { | |||||
| urlRuleDoc, | ||||||
| ), | ||||||
| ); | ||||||
|
|
||||||
| return `${EMOJI_DEPRECATED} This rule is deprecated.${ | ||||||
| replacedBy && replacedBy.length > 0 | ||||||
| ? ` It was replaced by ${replacementRuleList.join(', ')}.` | ||||||
|
|
@@ -277,7 +317,7 @@ function getNoticesForRule( | |||||
| configsError.length > 0 || | ||||||
| configsWarn.length > 0 || | ||||||
| configsOff.length > 0, | ||||||
| [NOTICE_TYPE.DEPRECATED]: rule.meta?.deprecated || false, | ||||||
| [NOTICE_TYPE.DEPRECATED]: Boolean(rule.meta?.deprecated) || false, | ||||||
| [NOTICE_TYPE.DESCRIPTION]: Boolean(rule.meta?.docs?.description) || false, | ||||||
|
|
||||||
| // Fixable/suggestions. | ||||||
|
|
@@ -359,6 +399,7 @@ function getRuleNoticeLines( | |||||
| configsOff, | ||||||
| ruleDocNotices, | ||||||
| ); | ||||||
|
|
||||||
| let noticeType: keyof typeof notices; | ||||||
|
|
||||||
| for (noticeType in notices) { | ||||||
|
|
@@ -392,7 +433,8 @@ function getRuleNoticeLines( | |||||
| fixable: Boolean(rule.meta?.fixable), | ||||||
| hasSuggestions: Boolean(rule.meta?.hasSuggestions), | ||||||
| urlConfigs, | ||||||
| replacedBy: rule.meta?.replacedBy, | ||||||
| deprecatedInfo: rule.meta?.deprecated, | ||||||
| replacedBy: rule.meta?.replacedBy, // eslint-disable-line @typescript-eslint/no-deprecated | ||||||
| plugin, | ||||||
| pluginPrefix, | ||||||
| pathPlugin, | ||||||
|
|
||||||
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.
Why is boolean included here?
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.
Typescript would complain about it when it's passed to
ruleNoticeStrOrFn(ts2322) becausemeta?.deprecatedstill includes thebooleantypeThere 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.
Let's call this
deprecatedto match the rule property name.