feat: Turn off rules that are not fixable

If we can't check if a rule is fixable or not, leave it turned on. If we find eslint 4.15.0 or later, use cliEngine's getRules, otherwise fallback to our set of known unfixable rules.
prettier · Jan 18, 2018 · 6ed4743 · 6ed4743
1 parent 0a8662c
commit 6ed4743
Show file tree

Hide file tree

Showing 3 changed files with 149 additions and 87 deletions.
diff --git a/src/__tests__/utils.js b/src/__tests__/utils.js
@@ -1,3 +1,4 @@
+import path from "path";
 import { getOptionsForFormatting } from "../utils";
 
 const getPrettierOptionsFromESLintRulesTests = [
@@ -177,6 +178,8 @@ const getPrettierOptionsFromESLintRulesTests = [
   { rules: { "arrow-parens": [0] }, options: {} }
 ];
 
+const eslintPath = path.join(__dirname, "../__mocks__/eslint");
+
 beforeEach(() => {
   global.__PRETTIER_ESLINT_TEST_STATE__ = {};
 });
@@ -187,7 +190,8 @@ getPrettierOptionsFromESLintRulesTests.forEach(
       const { prettier } = getOptionsForFormatting(
         { rules },
         prettierOptions,
-        fallbackPrettierOptions
+        fallbackPrettierOptions,
+        eslintPath
       );
       expect(prettier).toMatchObject(options);
     });
@@ -199,7 +203,9 @@ test("if prettierOptions are provided, those are preferred", () => {
     { rules: { quotes: [2, "single"] } },
     {
       singleQuote: false
-    }
+    },
+    undefined,
+    eslintPath
   );
   expect(prettier).toMatchObject({ singleQuote: false });
 });
@@ -215,30 +221,41 @@ test(`if fallbacks are provided, those are preferred over disabled eslint rules`
     {},
     {
       singleQuote: true
-    }
+    },
+    eslintPath
   );
   expect(prettier).toMatchObject({ singleQuote: true });
 });
 
 test("if fallbacks are provided, those are used if not found in eslint", () => {
-  const { prettier } = getOptionsForFormatting({ rules: {} }, undefined, {
-    singleQuote: false
-  });
+  const { prettier } = getOptionsForFormatting(
+    { rules: {} },
+    undefined,
+    {
+      singleQuote: false
+    },
+    eslintPath
+  );
   expect(prettier).toMatchObject({ singleQuote: false });
 });
 
 test("eslint max-len.tabWidth value should be used for tabWidth when tabs are used", () => {
-  const { prettier } = getOptionsForFormatting({
-    rules: {
-      indent: ["error", "tab"],
-      "max-len": [
-        2,
-        {
-          tabWidth: 4
-        }
-      ]
-    }
-  });
+  const { prettier } = getOptionsForFormatting(
+    {
+      rules: {
+        indent: ["error", "tab"],
+        "max-len": [
+          2,
+          {
+            tabWidth: 4
+          }
+        ]
+      }
+    },
+    undefined,
+    undefined,
+    eslintPath
+  );
 
   expect(prettier).toMatchObject({
     tabWidth: 4,
@@ -247,16 +264,21 @@ test("eslint max-len.tabWidth value should be used for tabWidth when tabs are us
 });
 
 test("Turn off prettier/prettier rule if found, but still infer options from it", () => {
-  const { eslint, prettier } = getOptionsForFormatting({
-    rules: {
-      "prettier/prettier": [
-        2,
-        {
-          trailingComma: "all"
-        }
-      ]
-    }
-  });
+  const { eslint, prettier } = getOptionsForFormatting(
+    {
+      rules: {
+        "prettier/prettier": [
+          2,
+          {
+            trailingComma: "all"
+          }
+        ]
+      }
+    },
+    undefined,
+    undefined,
+    eslintPath
+  );
 
   expect(eslint).toMatchObject({
     rules: {
@@ -270,10 +292,15 @@ test("Turn off prettier/prettier rule if found, but still infer options from it"
 });
 
 test("eslint config has only necessary properties", () => {
-  const { eslint } = getOptionsForFormatting({
-    globals: ["window:false"],
-    rules: { "no-with": "error", quotes: [2, "single"] }
-  });
+  const { eslint } = getOptionsForFormatting(
+    {
+      globals: ["window:false"],
+      rules: { "no-with": "error", quotes: [2, "single"] }
+    },
+    undefined,
+    undefined,
+    eslintPath
+  );
   expect(eslint).toMatchObject({
     fix: true,
     useEslintrc: false,
@@ -282,6 +309,36 @@ test("eslint config has only necessary properties", () => {
 });
 
 test("useEslintrc is set to the given config value", () => {
-  const { eslint } = getOptionsForFormatting({ useEslintrc: true, rules: {} });
+  const { eslint } = getOptionsForFormatting(
+    { useEslintrc: true, rules: {} },
+    undefined,
+    undefined,
+    eslintPath
+  );
   expect(eslint).toMatchObject({ fix: true, useEslintrc: true });
 });
+
+test("Turn off unfixable rules", () => {
+  const { eslint } = getOptionsForFormatting(
+    {
+      rules: {
+        "prettier/prettier": ["error"],
+        "valid-jsdoc": ["error"],
+        quotes: ["error", "double"]
+      }
+    },
+    undefined,
+    undefined,
+    eslintPath
+  );
+
+  expect(eslint).toMatchObject({
+    rules: {
+      "prettier/prettier": ["off"],
+      "valid-jsdoc": ["off"],
+      quotes: ["error", "double"]
+    },
+    fix: true,
+    useEslintrc: false
+  });
+});
diff --git a/src/index.js b/src/index.js
@@ -8,7 +8,11 @@ import { oneLine, stripIndent } from "common-tags";
 import indentString from "indent-string";
 import getLogger from "loglevel-colored-level-prefix";
 import merge from "lodash.merge";
-import { getOptionsForFormatting } from "./utils";
+import {
+  getESLintCLIEngine,
+  getOptionsForFormatting,
+  requireModule
+} from "./utils";
 
 const logger = getLogger({ prefix: "prettier-eslint" });
 
@@ -75,7 +79,8 @@ function format(options) {
   const formattingOptions = getOptionsForFormatting(
     eslintConfig,
     prettierOptions,
-    fallbackPrettierOptions
+    fallbackPrettierOptions,
+    eslintPath
   );
 
   logger.debug(
@@ -265,28 +270,3 @@ function getModulePath(filePath = __filename, moduleName) {
 function getDefaultLogLevel() {
   return process.env.LOG_LEVEL || "warn";
 }
-
-function requireModule(modulePath, name) {
-  try {
-    logger.trace(`requiring "${name}" module at "${modulePath}"`);
-    return require(modulePath);
-  } catch (error) {
-    logger.error(
-      oneLine`
-      There was trouble getting "${name}".
-      Is "${modulePath}" a correct path to the "${name}" module?
-    `
-    );
-    throw error;
-  }
-}
-
-function getESLintCLIEngine(eslintPath, eslintOptions) {
-  const { CLIEngine } = requireModule(eslintPath, "eslint");
-  try {
-    return new CLIEngine(eslintOptions);
-  } catch (error) {
-    logger.error(`There was trouble creating the ESLint CLIEngine.`);
-    throw error;
-  }
-}
diff --git a/src/utils.js b/src/utils.js
@@ -57,14 +57,15 @@ const OPTION_GETTERS = {
 };
 
 /* eslint import/prefer-default-export:0 */
-export { getOptionsForFormatting };
+export { getESLintCLIEngine, getOptionsForFormatting, requireModule };
 
 function getOptionsForFormatting(
   eslintConfig,
   prettierOptions = {},
-  fallbackPrettierOptions = {}
+  fallbackPrettierOptions = {},
+  eslintPath
 ) {
-  let eslint = getRelevantESLintConfig(eslintConfig);
+  let eslint = getRelevantESLintConfig(eslintConfig, eslintPath);
   const prettier = getPrettierOptionsFromESLintRules(
     eslintConfig,
     prettierOptions,
@@ -83,35 +84,34 @@ function getOptionsForFormatting(
   return { eslint, prettier };
 }
 
-function getRelevantESLintConfig(eslintConfig) {
+function getRelevantESLintConfig(eslintConfig, eslintPath) {
+  const cliEngine = getESLintCLIEngine(eslintPath);
+  // TODO: Actually test this branch
+  // istanbul ignore next
+  const loadedRules =
+    (cliEngine.getRules && cliEngine.getRules()) ||
+    new Map([
+      ["valid-jsdoc", { meta: {} }],
+      ["global-require", { meta: {} }],
+      ["no-with", { meta: {} }]
+    ]);
+
   const { rules } = eslintConfig;
-  // TODO: remove rules that are not fixable for perf
-  // this will require we load the config for every rule...
-  // not sure that'll be worth the effort
-  // but we may be able to maintain a manual list of rules that
-  // are definitely not fixable. Which is what we'll do for now...
-  const rulesThatWillNeverBeFixable = [
-    // TODO add more
-    "valid-jsdoc",
-    "global-require",
-    "no-with"
-  ];
-
-  logger.debug("reducing eslint rules down to relevant rules only");
+
+  logger.debug("turning off unfixable rules");
+
   const relevantRules = Object.entries(rules).reduce(
     (rulesAccumulator, [name, rule]) => {
-      if (rulesThatWillNeverBeFixable.includes(name)) {
-        logger.trace(
-          `omitting from relevant rules:`,
-          JSON.stringify({ [name]: rule })
-        );
-      } else {
-        logger.trace(
-          `adding to relevant rules:`,
-          JSON.stringify({ [name]: rule })
-        );
-        rulesAccumulator[name] = rule;
+      if (loadedRules.has(name)) {
+        const { meta: { fixable } } = loadedRules.get(name);
+
+        if (!fixable) {
+          logger.trace("turing off rule:", JSON.stringify({ [name]: rule }));
+          rule = ["off"];
+        }
       }
+
+      rulesAccumulator[name] = rule;
       return rulesAccumulator;
     },
     {}
@@ -409,3 +409,28 @@ function makePrettierOption(prettierRuleName, prettierRuleValue, fallbacks) {
   );
   return undefined;
 }
+
+function requireModule(modulePath, name) {
+  try {
+    logger.trace(`requiring "${name}" module at "${modulePath}"`);
+    return require(modulePath);
+  } catch (error) {
+    logger.error(
+      oneLine`
+      There was trouble getting "${name}".
+      Is "${modulePath}" a correct path to the "${name}" module?
+    `
+    );
+    throw error;
+  }
+}
+
+function getESLintCLIEngine(eslintPath, eslintOptions) {
+  const { CLIEngine } = requireModule(eslintPath, "eslint");
+  try {
+    return new CLIEngine(eslintOptions);
+  } catch (error) {
+    logger.error(`There was trouble creating the ESLint CLIEngine.`);
+    throw error;
+  }
+}