Mark deprecated rules as deprecated in their metadata #911
Conversation
| @@ -23,6 +23,20 @@ describe('all rule files should be exported by the plugin', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('deprecated rules', function() { | |||
| it('marks all deprecated rules as deprecated', function() { | |||
There was a problem hiding this comment.
I wonder if we should just use the deprecated property to dynamically build up the deprecatedRules list?
There was a problem hiding this comment.
lol i see this in your OP now. yeah i think it might be worth it - getAllRules().filter(x => x.deprecated) seems like it should be a viable approach
There was a problem hiding this comment.
We don't have a getAllRules() function here, but I'll take another run at this and see what I can come up with. I agree that the duplicated knowledge of which rules are deprecated is not ideal.
* Refactor the creation of the top-level module exports. `rules` becomes `allRules` and includes the deprecated rules. * `deprecatedRules` is now computed using the new `meta.deprecated` property. * The `all` config needs to filter out the deprecated rules. * Extracted some utility functions to remove duplication.
| var activeRulesConfig = configureAsError(activeRules); | ||
|
|
||
| var deprecatedRules = filterRules(allRules, function(rule) { | ||
| return rule.meta.deprecated; |
There was a problem hiding this comment.
this seems like the react/ prefix won't actually be there for the deprecated rules?
There was a problem hiding this comment.
I don't believe it needs to be. It wasn't before - deprecatedRules was defined directly as:
var deprecatedRules = {
'no-comment-textnodes': require('./lib/rules/no-comment-textnodes'),
'require-extension': require('./lib/rules/require-extension'),
'wrap-multilines': require('./lib/rules/wrap-multilines')
};The react/ prefix is only needed for activeRulesConfig, because those are used to define the all configuration, whereas rules and deprecatedRules both use the un-prefixed names.
There was a problem hiding this comment.
That seems odd to me but I believe you that the behavior won't be changed by this PR - so while I think all the exported rule name should have the prefix, that clearly is out of scope of this PR. Thanks for clarifying!
There was a problem hiding this comment.
Here's the relevant code from ESLint (from https://github.com/eslint/eslint/blob/master/lib/rules.js#L54-L63):
function importPlugin(plugin, pluginName) {
if (plugin.rules) {
Object.keys(plugin.rules).forEach(function(ruleId) {
const qualifiedRuleId = `${pluginName}/${ruleId}`,
rule = plugin.rules[ruleId];
define(qualifiedRuleId, rule);
});
}
}You can see that it takes the responsibility of prefixing the plugin name.
|
This LGTM. Let's get multiple collabs' eyes on it before this gets merged, however. |
|
Merged, thanks! |
Over on eslint-find-rules, there has been some discussion about being able to detect deprecated rules in order to not report them as unused and to report when they are still being used.
In order to implement those features, there needs to be a way to detect that a rule has been deprecated. Core ESLint rules that are deprecated are marked as such in their metadata.
This PR adds the same
deprecatedflag to the metadata of the deprecated rules in this plugin.It might be possible to make use of this metadata in
index.jsto automatically determine which rules are deprecated, rather than duplicating the information in two places. However, my attempts at this were not that clean and didn’t seem worth it. I could take another look if you think it worth it.