-
-
Notifications
You must be signed in to change notification settings - Fork 67
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: additional rule metadata for deprecations #124
base: main
Are you sure you want to change the base?
Conversation
type ReplacementKind = | ||
'moved' | // The rule has moved to another plugin if plugin is set, otherwise the rule is renamed in the same plugin | ||
'merged' | // The rule merged with another rule | ||
'option' // The current rule behavior is available as an option in the replacement rule |
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.
I think ReplacedByInfo.kind
isn't necessary as this info and more context can be provided as part of the ReplacedByInfo.info
(or ReplacedByInfo.info.message) text.
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.
The idea behind this property was:
- for documentation generators: able to provide a default "message" without having to specify the same message for multiple rules (e.g. the rule was renamed from "foo" to "bar")
- a possible config migration tool: suggest changes without relying on a specific format in info.message (e.g. rename the rule in the config)
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.
I'm also not sure of the value here. A single word just doesn't given enough context. I also fear this would become a maintenance issue as we'll end finding other reasons rules were deprecated and need to update this list with more unclear terms.
### Shorthand | ||
The shorthand for the properties `plugin`, `rule` and `info` is just a string representing either the `name`/`message` or the `url` based on its content. | ||
If it starts with a protocol (e.g. `https://`) the property should be interpreted as if only the `url` property is set, otherwise it should be interpreted as `name`/`message` property. | ||
This shorthand also applies for the existing `meta.deprecated` which then applies for the `meta.deprecated.info` properties. | ||
Some examples: | ||
```js | ||
{ meta: { deprecated: { plugin: 'https://eslint.style' } } } // <=> { meta: { deprecated: { plugin: { url: 'https://eslint.style' } } } } | ||
{ meta: { deprecated: { plugin: '@eslint-stylistic/js' } } } // <=> { meta: { deprecated: { plugin: { name: '@eslint-stylistic/js' } } } } | ||
{ meta: { deprecated: 'https://eslint.style/guide/migration' } // <=> { meta: { deprecated: { info: { url: 'https://eslint.style/guide/migration' } } } } | ||
``` |
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.
In my opinion, this is an unnecessary complexity that tooling would need to support. I'd be in favor of dropping shorthands to simplify this. Deprecations don't happen often, and it doesn't take much effort to write an object with a name
or url
property instead of a string.
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.
The primary motiviation for this were comments in the initial proposal how an empty object should be interpreted. With the shorthands, we can make all properties required instead of at least one property.
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.
Agree with @mdjermanovic on this.
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.
deprecated: {}
can be interpreted the same as deprecated: true
. Other properties can be interpreted as if they were not provided.
3. Should the exact regular expression for the shorthand which decides whether it is a description or URL be specified? | ||
4. Should the shorthand also be applied for the string form of the `meta.deprecated` property? |
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.
I'd prefer dropping shorthands, so we wouldn't have these questions.
5. Which "extension points" (rules, processors, configurations, parsers, formatters) shold be supported? | ||
6. Should the `rule` key be dependent on the "extension point" (e.g. `processor` for processors) or renamed (e.g. ``) so that it is the same property name for all? |
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.
I think the scope of this RFC is just rules deprecations. But if we are already anticipating that other extension points (processors, configurations, parsers, formatters) will have deprecation info, with the same shape, then a more generic name like replacement
might have sense, so that it's the same property name for all.
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.
I do not know of examples for other extension points being deprecated but especially with the introduction of language plugins, I can see some processors being deprecated (e.g. eslint-plugin-markdown).
}; | ||
|
||
/* At least one property is required */ | ||
type DeprecateInfo = { |
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.
type DeprecateInfo = { | |
type DeprecatedInfo = { |
kind?: ReplacementKind // Defaults to "moved" if missing | ||
} | ||
|
||
type Message = { |
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.
I'm not sure this type is necessary. I'd rather just move message
and url
to DeprecatedInfo
and ReplacedByInfo
. Remove one level of objects.
type ReplacementKind = | ||
'moved' | // The rule has moved to another plugin if plugin is set, otherwise the rule is renamed in the same plugin | ||
'merged' | // The rule merged with another rule | ||
'option' // The current rule behavior is available as an option in the replacement rule |
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.
I'm also not sure of the value here. A single word just doesn't given enough context. I also fear this would become a maintenance issue as we'll end finding other reasons rules were deprecated and need to update this list with more unclear terms.
rule: 'https://eslint.style/rules/js/semi', | ||
}, |
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.
Syntax error
message: 'Stylistic rules are being moved out of ESLint core.', | ||
url: 'https://eslint.org/blog/2023/10/deprecating-formatting-rules/', |
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.
message
and url
aren't listed as members above (although, I do like this better. See my earlier comments.)
### Shorthand | ||
The shorthand for the properties `plugin`, `rule` and `info` is just a string representing either the `name`/`message` or the `url` based on its content. | ||
If it starts with a protocol (e.g. `https://`) the property should be interpreted as if only the `url` property is set, otherwise it should be interpreted as `name`/`message` property. | ||
This shorthand also applies for the existing `meta.deprecated` which then applies for the `meta.deprecated.info` properties. | ||
Some examples: | ||
```js | ||
{ meta: { deprecated: { plugin: 'https://eslint.style' } } } // <=> { meta: { deprecated: { plugin: { url: 'https://eslint.style' } } } } | ||
{ meta: { deprecated: { plugin: '@eslint-stylistic/js' } } } // <=> { meta: { deprecated: { plugin: { name: '@eslint-stylistic/js' } } } } | ||
{ meta: { deprecated: 'https://eslint.style/guide/migration' } // <=> { meta: { deprecated: { info: { url: 'https://eslint.style/guide/migration' } } } } | ||
``` |
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.
Agree with @mdjermanovic on this.
--> | ||
|
||
1. Is there additional deprecation information we'd like to represent? Note that additional information can always be added later, but it's good to consider any possible needs now. | ||
2. Should `meta.deprecated.plugin.id` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) |
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.
2. Should `meta.deprecated.plugin.id` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) | |
2. Should `meta.deprecated.plugin.name` accommodate different package registries (e.g. [jsr](https://jsr.io/) with `jsr:eslint-plugin-example`) |
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.
In general, I don't think we should worry about package registries. People should include a URL to the package wherever its published if this is a concern.
deprecatedSince?: Version // Helps users gauge when to migrate and useful for documentation | ||
availableUntil?: Version | null // The estimated version when the rule is removed (probably the next major version). null means the rule is "frozen" (will be available but will not be changed) |
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.
Will the Version
format be the same as the value as in package.json, or will it include a v
in the beginning (e.g. "v9.11.1"
)?
Also, let's suppose that we decide to deprecate a rule in ESLint v10. What value should be set for deprecatedSince
? Should it be "v10.0.0-alpha.0"
,"v10.0.0"
, "v10"
, etc.?
I believe in practice every plugin will end up using these fields slightly differently, but it would be useful to include usage examples for ESLint specifically.
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.
Also, let's suppose that we decide to deprecate a rule in ESLint v10. What value should be set for
deprecatedSince
? Should it be"v10.0.0-alpha.0"
,"v10.0.0"
,"v10"
, etc.?
10.0.0
. Prereleases don't count and we should use the full semver version.
Summary
This is a continuation of #116 by @bmish to specify additional properties for rule deprecations.
The main changes are:
meta.deprecated
and deprecatemeta.replacedBy
kind
,deprecatedSince
,availableUntil
Related Issues