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

Mark deprecated rules as deprecated in their metadata #911

Merged
merged 2 commits into from Nov 1, 2016

Conversation

5 participants
@randycoulman
Copy link
Contributor

randycoulman commented Oct 14, 2016

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 deprecated flag to the metadata of the deprecated rules in this plugin.

It might be possible to make use of this metadata in index.js to 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.

@ljharb

ljharb approved these changes Oct 15, 2016

Copy link
Collaborator

ljharb left a comment

LGTM + comment

@@ -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() {

This comment has been minimized.

@ljharb

ljharb Oct 15, 2016

Collaborator

I wonder if we should just use the deprecated property to dynamically build up the deprecatedRules list?

This comment has been minimized.

@ljharb

ljharb Oct 15, 2016

Collaborator

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

This comment has been minimized.

@randycoulman

randycoulman Oct 15, 2016

Contributor

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.

This comment has been minimized.

@randycoulman

randycoulman Oct 15, 2016

Contributor

@ljharb OK, here's what I came up with: 33aa4d5. Let me know what you think.

Compute deprecatedRules from deprecated meta property
* 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;

This comment has been minimized.

@ljharb

ljharb Oct 15, 2016

Collaborator

this seems like the react/ prefix won't actually be there for the deprecated rules?

This comment has been minimized.

@randycoulman

randycoulman Oct 15, 2016

Contributor

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.

This comment has been minimized.

@ljharb

ljharb Oct 15, 2016

Collaborator

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!

This comment has been minimized.

@randycoulman

randycoulman Oct 15, 2016

Contributor

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 comment has been minimized.

@ljharb

ljharb Oct 15, 2016

Collaborator

hm, that's a good point. thanks

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 15, 2016

This LGTM. Let's get multiple collabs' eyes on it before this gets merged, however.

@yannickcr yannickcr merged commit 6620a0a into yannickcr:master Nov 1, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 97.126%
Details
@yannickcr

This comment has been minimized.

Copy link
Owner

yannickcr commented Nov 1, 2016

Merged, thanks!

@randycoulman randycoulman deleted the randycoulman:include-deprecation-in-metadata branch Jul 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment