Rule Blocklist #231

pointlessone · 2017-03-22T16:39:17Z

Add a way to disable rules.

One particular case is #222.

pbrisbin · 2017-03-22T16:41:21Z

lib/eslint-patch.js

@@ -7,6 +7,7 @@ const ConfigFile = require("eslint/lib/config/config-file");

 const Config = require("eslint/lib/config");
 const ConfigUpgrader = require("./config_upgrader");
+const RuleBlacklist = require("./rule_blacklist");


Could you rename your files and variables to use "blocklist" throughout?

pbrisbin · 2017-03-22T17:01:47Z

lib/rule_blocklist.js

+  let report = [];
+
+  for (const name in blocklistedRules) {
+    if (Reflect.has(rules, name)) {


Just curious, what's Reflect? I don't see it being required.

It's a standard built-in object.

pbrisbin · 2017-03-22T17:03:01Z

lib/config_finder.js

+"use strict";
+
+const
+    CONFIG_FILES = require("eslint/lib/config/config-file").CONFIG_FILES


💄 not sure what project style is, but elsewhere you put the first value on the line with const.

pbrisbin · 2017-03-22T17:03:17Z

lib/rule_blocklist.js

@@ -0,0 +1,84 @@
+"use strict";
+
+const a = "a"


What's a for?

dblandin · 2017-03-22T17:41:08Z

This is looking good! The current rule configured in the blocklist is missing the import/ prefix, but after fixing that the engine successfully ignored the rule. 🎉

Instead of

Ignored rules:

* import/no-unresolved:
  Code Climate doesn't fetch your project's dependencies so this rule produces high amount of false positives.

what do you think of outputting a list of ignored rules (if any) after a brief explanation:

Ignoring the following rules that rely on module resolution:

* import/no-unresolved

dblandin · 2017-03-22T17:59:14Z

lib/rule_blocklist.js

+
+const blocklistedRules = {
+  "no-unresolved": "Code Climate doesn't fetch your project's dependencies so this rule produces high amount of false positives."
+};


Mentioned in another comment but repeating here inline: I don't think we need to support separate explanations for blocklisted rules just yet. This can be an array of rules and we can provide an explanation after updating the config.

no-resolved should be import/no-resolved.

Ignoring the following rules that rely on module resolution: * import/no-unresolved

Right now we have just one reason (we know of) to turn off rules. And we have just one rule (I didn't do extensive research on this yet). It's an appropriate way to format the message for it.

However, it doesn't seem to be the only possible reason. I specifically chosen this approach because there might be others and it will be a bit easier to change it in the future — just add it to this object, no code changes needed.

Here are a few examples of other reasons we may want to disable a rule I can think of:

A platform-specific rule that is incompatible with Linux

A rule that relies on an external tool that is not in the container

A rule that tries to use network

A rule that is incompatible with Node version we run

I'm not opposing the proposed change. I'm just presenting my reasoning behind the design. I'll make the change if you prefer it.

We should definitely audit all rules currently available via our vendored plugins and add any that rely on module resolution to this blocklist either in this PR or the next one.

Here's one example of another rule that should be in the blocklist:
import/extensions
https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/extensions.js

Here are a few examples of other reasons we may want to disable a rule I can think of

Until we actually want to ignore rules for another reason, I'd stick with a simpler implementation that doesn't map particular rules to warning messages. I don't think it's worth adding now. If we need it later, we can add it then.

dblandin · 2017-03-30T17:10:47Z

@pointlessone Anything I can do to help push this along?

efueger · 2017-04-03T16:50:05Z

Another rule that should probably be included in the blocklist:

import/no-absolute-path

Otherwise, repos see issues like: "Resolve error: unable to load resolver "babel-module"."

dblandin · 2017-04-04T15:49:18Z

@pointlessone Looks like a good portion of the diff is refactoring the config finder. Would you mind splitting that refactor into a separate commit so the rule blocklist additions can stand alone?

dblandin · 2017-04-04T15:52:17Z

@pointlessone Have you been able to audit the currently vendored plugins for other rules that rely on module resolution? It would be great if we could catch and blocklist any others.

pointlessone · 2017-04-04T17:56:48Z

@dblandin No, I haven't audited other rules. It's a rather time consuming task. I'm not sure if that's an effective use of time. I thought we could push this and add more rules as we hit more false positives. /cc @gordondiggs

TBH, I'm not sure about import/extensions rule either. According to docs it seem only deal with extensions and shouldn't rely on module resolution. On the other hand, implementation seem to use resolve. ¯\_(ツ)_/¯

dblandin · 2017-04-04T18:12:27Z

I'm fine rolling with this if you think an audit is too time intensive right right now.

I was thinking of a simple ack/grep for rules calling out to resolve and checking each briefly for module resolution:

$ ack "resolve" ./**/rules/**/*.js | grep ".js$"

eslint-config-airbnb-base/rules/imports.js
eslint-config-airbnb-base/rules/style.js
eslint-config-airbnb/rules/react.js
eslint-config-simplifield/node_modules/eslint-plugin-angular/rules/deferred.js
eslint/lib/rules/global-require.js
eslint/lib/rules/no-alert.js
eslint/lib/rules/no-loop-func.js
eslint/lib/rules/no-path-concat.js
eslint/lib/rules/no-unmodified-loop-condition.js
eslint/lib/rules/no-unused-vars.js
eslint/lib/rules/no-use-before-define.js
eslint/lib/rules/prefer-const.js
eslint/lib/rules/prefer-promise-reject-errors.js
eslint-plugin-angular/rules/deferred.js
eslint-plugin-filenames/lib/rules/match-exported.js
eslint-plugin-filenames/lib/rules/match-regex.js
eslint-plugin-filenames/lib/rules/no-index.js
eslint-plugin-flowtype/dist/rules/defineFlowType.js
eslint-plugin-import/lib/rules/extensions.js
eslint-plugin-import/lib/rules/no-duplicates.js
eslint-plugin-import/lib/rules/no-internal-modules.js
eslint-plugin-import/lib/rules/no-restricted-paths.js
eslint-plugin-import/lib/rules/no-unresolved.js
eslint-plugin-inferno/lib/rules/no-unused-prop-types.js
eslint-plugin-inferno/lib/rules/prop-types.js
eslint-plugin-jsx-a11y/lib/rules/aria-proptypes.js
eslint-plugin-jsx-a11y/src/rules/aria-proptypes.js
eslint-plugin-node/lib/rules/no-hide-core-modules.js
eslint-plugin-node/lib/rules/no-unpublished-bin.js
eslint-plugin-node/lib/rules/shebang.js
eslint-plugin-promise/rules/lib/is-promise.js
eslint-plugin-promise/rules/no-return-wrap.js
eslint-plugin-promise/rules/param-names.js
eslint-plugin-react/lib/rules/no-unused-prop-types.js
eslint-plugin-react/lib/rules/prop-types.js
eslint-plugin-react/lib/rules/require-default-props.js

pointlessone · 2017-04-05T13:31:37Z

@dblandin Split the commits.

Simple grep is a rather naive approach. I imagine it can hit all sorts of unrelated things: comments, any function/var that has this rather generic term in them, etc.

Moreover, not every resolve is destined to hit external modules. After all there's some code in the project.

Let's merge this PR as is (or with the changes directly related to it) and expand the list of blocked rules in a separate PR. I'll go through the list you posted and add rules if they primarily deal with external modules. Otherwise, users can mark false-positives.

dblandin · 2017-04-05T13:45:15Z

Sounds good!

codeclimate-hermes assigned gdiggs Mar 22, 2017

pointlessone requested a review from dblandin March 22, 2017 16:40

pbrisbin reviewed Mar 22, 2017

View reviewed changes

pointlessone force-pushed the rule-blacklist branch from 3b513c4 to 41e4720 Compare March 22, 2017 16:54

pointlessone changed the title ~~Rule Blacklist~~ Rule Blocklist Mar 22, 2017

pointlessone force-pushed the rule-blacklist branch from 41e4720 to 63f2b0e Compare March 22, 2017 16:55

pbrisbin reviewed Mar 22, 2017

View reviewed changes

lib/rule_blocklist.js Outdated

@@ -0,0 +1,84 @@

"use strict";

const a = "a"

Copy link

Contributor

pbrisbin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a for?

dblandin reviewed Mar 22, 2017

View reviewed changes

pointlessone force-pushed the rule-blacklist branch from 63f2b0e to 9356897 Compare March 23, 2017 10:07

pointlessone force-pushed the rule-blacklist branch from 9356897 to 715c202 Compare April 4, 2017 11:34

pointlessone requested a review from dblandin April 4, 2017 12:03

pointlessone added 2 commits April 5, 2017 16:21

Extract config finder

859d128

Add Rule Blocklist

1d9e86c

pointlessone force-pushed the rule-blacklist branch from 715c202 to 1d9e86c Compare April 5, 2017 13:21

dblandin approved these changes Apr 5, 2017

View reviewed changes

pointlessone merged commit 7c0ac0a into master Apr 5, 2017

pointlessone deleted the rule-blacklist branch April 5, 2017 13:48

dblandin mentioned this pull request Apr 6, 2017

Ignore ESLint rules that rely on module resolution #222

Closed

pointlessone mentioned this pull request Apr 10, 2017

Rule Blocklist update #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule Blocklist #231

Rule Blocklist #231

pointlessone commented Mar 22, 2017

pbrisbin Mar 22, 2017

pbrisbin Mar 22, 2017

pointlessone Mar 23, 2017

pbrisbin Mar 22, 2017

pbrisbin Mar 22, 2017

dblandin commented Mar 22, 2017

dblandin Mar 22, 2017

pointlessone Mar 23, 2017

dblandin Mar 23, 2017

dblandin commented Mar 30, 2017

efueger commented Apr 3, 2017

dblandin commented Apr 4, 2017

dblandin commented Apr 4, 2017

pointlessone commented Apr 4, 2017

dblandin commented Apr 4, 2017

pointlessone commented Apr 5, 2017

dblandin commented Apr 5, 2017

Rule Blocklist #231

Rule Blocklist #231

Conversation

pointlessone commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblandin commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblandin commented Mar 30, 2017

efueger commented Apr 3, 2017

dblandin commented Apr 4, 2017

dblandin commented Apr 4, 2017

pointlessone commented Apr 4, 2017

dblandin commented Apr 4, 2017

pointlessone commented Apr 5, 2017

dblandin commented Apr 5, 2017