Repository: eslint/eslint
Pull request #8198 (2017-03-04 03:03:13)

Chore: add eslint-plugin-eslint-plugin
**What is the purpose of this pull request? (put an "X" next to item)**

[x] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

This adds [eslint-plugin-eslint-plugin](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin) for linting some of our rules and tests.

The following rules are enabled:

* [no-deprecated-report-api](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/no-deprecated-report-api.md) prohibits the deprecated `context.report(node, message)` API. (The autofixer for this rule was used in https://github.com/eslint/eslint/pull/7763.)
* [report-message-format](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/report-message-format.md) enforces a consistent format for report messages. I configured this to enforce that report messages start with a capital letter and end with a period, because almost all of our rules follow this convention.
* [require-meta-fixable](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/require-meta-fixable.md) requires valid `fix:` metadata for fixable rules. Previously, one of rules was specifying `fix: false` which isn't a valid value based on our docs.
* [prefer-placeholders](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/prefer-placeholders.md) enforces that rules use `{{}}` placeholders as report messages, rather than string concatenation.
* [no-missing-placeholders](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/no-missing-placeholders.md) enforces that report message placeholders are valid.
* [no-useless-token-range](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/no-useless-token-range.md) disallows code like `sourceCode.getFirstToken(node).range[0]` in favor of `node.range[0]`, since the two are identical.
* [consistent-output](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/consistent-output.md) enforces that rule tests must specify an output assertion for all test cases, if any test cases have output assertions. (This would have prevented the missing coverage that was solved by https://github.com/eslint/eslint/pull/8129 and https://github.com/eslint/eslint/pull/8195.)

This also removes part of the `internal-no-invalid-meta` logic which is covered by `require-meta-fixable`.

**Is there anything you'd like reviewers to focus on?**

* Do we want to add this?
* Do we want to enable any other rules? The one rule from the plugin that this PR does not enable is [test-case-shorthand-strings](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/test-case-shorthand-strings.md), which enforces a consistent style for valid test cases with no options.

eslintbot: LGTM
mention-bot: @not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @btmills and @mysticatea to be potential reviewers.
ilyavolodin: I think it's reasonable to include this in. Especially for the fixable rule meta enforcement.
vitorbal: Whats the difference between the plugin and the internal rules? Would it be
possible to merge them together?
On Sat, Mar 4, 2017 at 2:44 PM Gyandeep Singh <notifications@github.com>
wrote:

> *@gyandeeps* approved this pull request.
>
> —
> You are receiving this because you were mentioned.
>
>
> Reply to this email directly, view it on GitHub
> <https://github.com/eslint/eslint/pull/8198#pullrequestreview-25128434>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAmNdgbLmacX6SppMnG6UCYb4fPiYp9mks5riaLngaJpZM4MS89L>
> .
>

not-an-aardvark: > Whats the difference between the plugin and the internal rules? Would it be
possible to merge them together?

The internal rules also enforce that the `meta.docs.description` and `meta.docs.category` properties exist. Another internal rule verifies that `meta.docs.description` follows our conventions.
vitorbal: Could we, for instance, move all those internal rules into
eslint-plugin-eslint-plugin? I imagine not all plugin authors would want to
use it, but that way we simplify the internals of eslint a bit, while still
dogfooding those rules.
On Sat, Mar 4, 2017 at 3:23 PM Teddy Katz <notifications@github.com> wrote:

> Whats the difference between the plugin and the internal rules? Would it be
> possible to merge them together?
>
> The internal rules also enforce that the meta.docs.description and
> meta.docs.category properties exist. Another internal rule verifies that
> meta.docs.description follows our conventions.
>
> —
> You are receiving this because you were mentioned.
>
>
> Reply to this email directly, view it on GitHub
> <https://github.com/eslint/eslint/pull/8198#issuecomment-284170498>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAmNduFOI4fU_c0SjKF4DQ52D6TCZI3Uks5riawpgaJpZM4MS89L>
> .
>

not-an-aardvark: > Could we, for instance, move all those internal rules into
eslint-plugin-eslint-plugin? I imagine not all plugin authors would want to
use it, but that way we simplify the internals of eslint a bit, while still
dogfooding those rules.

I suppose we could do that, but the goal of the plugin is to have rules that are generally applicable to plugin authors. :smiley: If we have very specific things to enforce, I think it's reasonable for us to keep some internal rules.
vitorbal: Fair enough!

Patch:
diff --git a/.eslintrc.yml b/.eslintrc.yml
index 0f505ebc7..c857caa67 100644
--- a/.eslintrc.yml
+++ b/.eslintrc.yml
@@ -1,7 +1,13 @@
 root: true
 
 plugins:
+    - eslint-plugin
     - node
 extends:
     - "./packages/eslint-config-eslint/default.yml"
     - "plugin:node/recommended"
+    - "plugin:eslint-plugin/recommended"
+rules:
+    eslint-plugin/consistent-output: "error"
+    eslint-plugin/prefer-placeholders: "error"
+    eslint-plugin/report-message-format: ["error", '[^a-z].*\.$']
diff --git a/lib/internal-rules/internal-no-invalid-meta.js b/lib/internal-rules/internal-no-invalid-meta.js
index d1c78efa6..d13df358b 100644
--- a/lib/internal-rules/internal-no-invalid-meta.js
+++ b/lib/internal-rules/internal-no-invalid-meta.js
@@ -94,16 +94,6 @@ function hasMetaSchema(metaPropertyNode) {
     return getPropertyFromObject("schema", metaPropertyNode.value);
 }
 
-/**
- * Whether this `meta` ObjectExpression has a `fixable` property defined or not.
- *
- * @param {ASTNode} metaPropertyNode The `meta` ObjectExpression for this rule.
- * @returns {boolean} `true` if a `fixable` property exists.
- */
-function hasMetaFixable(metaPropertyNode) {
-    return getPropertyFromObject("fixable", metaPropertyNode.value);
-}
-
 /**
  * Checks the validity of the meta definition of this rule and reports any errors found.
  *
@@ -112,7 +102,7 @@ function hasMetaFixable(metaPropertyNode) {
  * @param {boolean} ruleIsFixable whether the rule is fixable or not.
  * @returns {void}
  */
-function checkMetaValidity(context, exportsNode, ruleIsFixable) {
+function checkMetaValidity(context, exportsNode) {
     const metaProperty = getMetaPropertyFromExportsNode(exportsNode);
 
     if (!metaProperty) {
@@ -142,11 +132,6 @@ function checkMetaValidity(context, exportsNode, ruleIsFixable) {
 
     if (!hasMetaSchema(metaProperty)) {
         context.report(metaProperty, "Rule is missing a meta.schema property.");
-        return;
-    }
-
-    if (ruleIsFixable && !hasMetaFixable(metaProperty)) {
-        context.report(metaProperty, "Rule is fixable, but is missing a meta.fixable property.");
     }
 }
 
@@ -177,7 +162,6 @@ module.exports = {
 
     create(context) {
         let exportsNode;
-        let ruleIsFixable = false;
 
         return {
             AssignmentExpression(node) {
@@ -191,35 +175,13 @@ module.exports = {
                 }
             },
 
-            CallExpression(node) {
-
-                // If the rule has a call for `context.report` and a property `fix`
-                // is being passed in, then we consider that the rule is fixable.
-                //
-                // Note that we only look for context.report() calls in the new
-                // style (with single MessageDescriptor argument), because only
-                // calls in the new style can specify a fix.
-                if (node.callee.type === "MemberExpression" &&
-                    node.callee.object.type === "Identifier" &&
-                    node.callee.object.name === "context" &&
-                    node.callee.property.type === "Identifier" &&
-                    node.callee.property.name === "report" &&
-                    node.arguments.length === 1 &&
-                    node.arguments[0].type === "ObjectExpression") {
-
-                    if (getPropertyFromObject("fix", node.arguments[0])) {
-                        ruleIsFixable = true;
-                    }
-                }
-            },
-
             "Program:exit"() {
                 if (!isCorrectExportsFormat(exportsNode)) {
                     context.report({ node: exportsNode, message: "Rule does not export an Object. Make sure the rule follows the new rule format." });
                     return;
                 }
 
-                checkMetaValidity(context, exportsNode, ruleIsFixable);
+                checkMetaValidity(context, exportsNode);
             }
         };
     }
diff --git a/lib/rules/no-invalid-regexp.js b/lib/rules/no-invalid-regexp.js
index dcde234c6..45596f7ee 100644
--- a/lib/rules/no-invalid-regexp.js
+++ b/lib/rules/no-invalid-regexp.js
@@ -74,7 +74,8 @@ module.exports = {
                 } catch (e) {
                     context.report({
                         node,
-                        message: `${e.message}.`
+                        message: "{{message}}.",
+                        data: e
                     });
                 }
 
diff --git a/lib/rules/no-restricted-properties.js b/lib/rules/no-restricted-properties.js
index b6c584c57..e3a3d14e5 100644
--- a/lib/rules/no-restricted-properties.js
+++ b/lib/rules/no-restricted-properties.js
@@ -109,6 +109,7 @@ module.exports = {
             if (matchedObjectProperty) {
                 const message = matchedObjectProperty.message ? ` ${matchedObjectProperty.message}` : "";
 
+                // eslint-disable-next-line eslint-plugin/report-message-format
                 context.report({ node, message: "'{{objectName}}.{{propertyName}}' is restricted from being used.{{message}}", data: {
                     objectName,
                     propertyName,
@@ -117,6 +118,7 @@ module.exports = {
             } else if (globalMatchedProperty) {
                 const message = globalMatchedProperty.message ? ` ${globalMatchedProperty.message}` : "";
 
+                // eslint-disable-next-line eslint-plugin/report-message-format
                 context.report({ node, message: "'{{propertyName}}' is restricted from being used.{{message}}", data: {
                     propertyName,
                     message
diff --git a/lib/rules/no-return-await.js b/lib/rules/no-return-await.js
index 3b7324d8b..a7933460d 100644
--- a/lib/rules/no-return-await.js
+++ b/lib/rules/no-return-await.js
@@ -19,7 +19,7 @@ module.exports = {
             category: "Best Practices",
             recommended: false // TODO: set to true
         },
-        fixable: false,
+        fixable: null,
         schema: [
         ]
     },
diff --git a/lib/rules/object-shorthand.js b/lib/rules/object-shorthand.js
index 7c4275ffe..1d32d1722 100644
--- a/lib/rules/object-shorthand.js
+++ b/lib/rules/object-shorthand.js
@@ -368,11 +368,12 @@ module.exports = {
                 // Checks for property/method shorthand.
                 if (isConciseProperty) {
                     if (node.method && (APPLY_NEVER || AVOID_QUOTES && isStringLiteral(node.key))) {
+                        const message = APPLY_NEVER ? "Expected longform method syntax." : "Expected longform method syntax for string literal keys.";
 
                         // { x() {} } should be written as { x: function() {} }
                         context.report({
                             node,
-                            message: `Expected longform method syntax${APPLY_NEVER ? "" : " for string literal keys"}.`,
+                            message,
                             fix: fixer => makeFunctionLongform(fixer, node)
                         });
                     } else if (APPLY_NEVER) {
diff --git a/lib/rules/prefer-destructuring.js b/lib/rules/prefer-destructuring.js
index 38e8e2903..d7fbc3559 100644
--- a/lib/rules/prefer-destructuring.js
+++ b/lib/rules/prefer-destructuring.js
@@ -89,7 +89,7 @@ module.exports = {
          * @returns {void}
          */
         function report(reportNode, type) {
-            context.report({ node: reportNode, message: `Use ${type} destructuring` });
+            context.report({ node: reportNode, message: "Use {{type}} destructuring.", data: { type } });
         }
 
         /**
diff --git a/lib/rules/yoda.js b/lib/rules/yoda.js
index 1a79522e7..bdb7d2a42 100644
--- a/lib/rules/yoda.js
+++ b/lib/rules/yoda.js
@@ -267,8 +267,8 @@ module.exports = {
             const operatorToken = sourceCode.getFirstTokenBetween(node.left, node.right, token => token.value === node.operator);
             const textBeforeOperator = sourceCode.getText().slice(sourceCode.getTokenBefore(operatorToken).range[1], operatorToken.range[0]);
             const textAfterOperator = sourceCode.getText().slice(operatorToken.range[1], sourceCode.getTokenAfter(operatorToken).range[0]);
-            const leftText = sourceCode.getText().slice(sourceCode.getFirstToken(node).range[0], sourceCode.getTokenBefore(operatorToken).range[1]);
-            const rightText = sourceCode.getText().slice(sourceCode.getTokenAfter(operatorToken).range[0], sourceCode.getLastToken(node).range[1]);
+            const leftText = sourceCode.getText().slice(node.range[0], sourceCode.getTokenBefore(operatorToken).range[1]);
+            const rightText = sourceCode.getText().slice(sourceCode.getTokenAfter(operatorToken).range[0], node.range[1]);
 
             return rightText + textBeforeOperator + OPERATOR_FLIP_MAP[operatorToken.value] + textAfterOperator + leftText;
         }
diff --git a/package.json b/package.json
index 72df096bc..cdd8cc0dc 100644
--- a/package.json
+++ b/package.json
@@ -81,6 +81,7 @@
     "coveralls": "^2.11.16",
     "dateformat": "^1.0.8",
     "ejs": "^2.3.3",
+    "eslint-plugin-eslint-plugin": "^0.7.1",
     "eslint-plugin-node": "^4.1.0",
     "eslint-release": "^0.10.0",
     "esprima": "^2.4.1",


Test patch:
diff --git a/tests/lib/internal-rules/internal-no-invalid-meta.js b/tests/lib/internal-rules/internal-no-invalid-meta.js
index 068e3f54a..6dd44ea2f 100644
--- a/tests/lib/internal-rules/internal-no-invalid-meta.js
+++ b/tests/lib/internal-rules/internal-no-invalid-meta.js
@@ -241,37 +241,6 @@ ruleTester.run("internal-no-invalid-meta", rule, {
                 line: 2,
                 column: 5
             }]
-        },
-        {
-            code: [
-                "module.exports = {",
-                "    meta: {",
-                "        docs: {",
-                "            description: 'some rule',",
-                "            category: 'Internal',",
-                "            recommended: false",
-                "        },",
-                "        schema: []",
-                "    },",
-                "    create: function(context) {",
-                "        return {",
-                "            Program: function(node) {",
-                "                context.report({",
-                "                    node: node,",
-                "                    fix: function(fixer) {",
-                "                        return fixer.insertTextAfter(node, ' ');",
-                "                    }",
-                "                });",
-                "            }",
-                "        };",
-                "    }",
-                "};"
-            ].join("\n"),
-            errors: [{
-                message: "Rule is fixable, but is missing a meta.fixable property.",
-                line: 2,
-                column: 5
-            }]
         }
     ]
 });
diff --git a/tests/lib/rules/prefer-destructuring.js b/tests/lib/rules/prefer-destructuring.js
index ae0633408..54cd76d0d 100644
--- a/tests/lib/rules/prefer-destructuring.js
+++ b/tests/lib/rules/prefer-destructuring.js
@@ -81,21 +81,21 @@ ruleTester.run("prefer-destructuring", rule, {
         {
             code: "var foo = array[0];",
             errors: [{
-                message: "Use array destructuring",
+                message: "Use array destructuring.",
                 type: "VariableDeclarator"
             }]
         },
         {
             code: "foo = array[0];",
             errors: [{
-                message: "Use array destructuring",
+                message: "Use array destructuring.",
                 type: "AssignmentExpression"
             }]
         },
         {
             code: "var foo = object.foo;",
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "VariableDeclarator"
             }]
         },
@@ -103,7 +103,7 @@ ruleTester.run("prefer-destructuring", rule, {
             code: "var foobar = object.bar;",
             options: [{ object: true }, { enforceForRenamedProperties: true }],
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "VariableDeclarator"
             }]
         },
@@ -111,28 +111,28 @@ ruleTester.run("prefer-destructuring", rule, {
             code: "var foo = object[bar];",
             options: [{ object: true }, { enforceForRenamedProperties: true }],
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "VariableDeclarator"
             }]
         },
         {
             code: "var foo = array['foo'];",
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "VariableDeclarator"
             }]
         },
         {
             code: "foo = array.foo;",
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "AssignmentExpression"
             }]
         },
         {
             code: "foo = array['foo'];",
             errors: [{
-                message: "Use object destructuring",
+                message: "Use object destructuring.",
                 type: "AssignmentExpression"
             }]
         }