Added obsolete rule support to convertRules.ts (#1148)

Josh Goldberg · web-flow · commit 84b22ce91d4f · 2021-07-06T10:29:16.000-04:00
diff --git a/docs/Architecture/Linters.md b/docs/Architecture/Linters.md
@@ -18,7 +18,7 @@ Those are run by `src/converters/lintConfigs/rules/convertRules.ts`, which takes
 
 1. The raw TSLint rule is converted to a standardized format.
 2. The appropriate converter is run for the rule.
-3. If the rule is missing or the conversion failed, this is marked.
+3. If the rule is missing or obsolete, or the conversion failed, this is marked.
 4. For each output rule equivalent given by the conversion:
     * The output rule name is added to the TSLint rule's equivalency set.
     * The TSLint rule's config severity is mapped to its ESLint equivalent.
@@ -34,7 +34,7 @@ Each TSLint rule should output at least one ESLint rule as the equivalent.
 
 Each converter for a TSLint rule takes an arguments object for the rule, and returns an array of objects containing:
 
--   `rules`: At least one equivalent ESLint rule and options
+-   `rules`: At least one equivalent ESLint rule and options, _or_ none if obsolete
 -   `notices`: Any extra info that should be printed after conversion
 -   `plugins`: Any plugins that should now be installed if not already
 
diff --git a/docs/Creating a Rule Converter.md b/docs/Creating a Rule Converter.md
@@ -14,3 +14,9 @@ If the lint rule includes arguments, add the `--sameArguments` flag above to hav
 ```shell
 node ./script/newConverter --eslint output-name --tslint input-name --sameArguments
 ```
+
+If the original TSLint rule is obsolete and does not have an ESLint equivalent, you can omit `--eslint`:
+
+```shell
+node ./script/newConverter --tslint input-name
+```
diff --git a/script/newConverter/index.js b/script/newConverter/index.js
@@ -13,17 +13,20 @@ const { writeConverterTest } = require("./writeConverterTest");
 
     const args = command.parse(process.argv).opts();
 
-    for (const arg of ["eslint", "tslint"]) {
-        if (!args[arg]) {
-            throw new Error(`Missing --${arg} option.`);
-        }
+    if (!args.tslint) {
+        throw new Error(`Missing --tslint option.`);
+    }
+
+    if (args.sameArguments && !args.eslint) {
+        throw new Error(`Cannot use --sameArguments without --eslint.`);
     }
 
     const tslintPascalCase = upperFirst(camelCase(args.tslint)).replace("A11Y", "A11y");
-    const plugins = args.eslint.includes("/")
-        ? `
+    const plugins =
+        args.eslint && args.eslint.includes("/")
+            ? `
         plugins: ["${args.eslint.split("/")[0]}"],`
-        : "";
+            : "";
 
     await rewriteConvertersMap({ args, tslintPascalCase });
     await writeConverter({ args, plugins, tslintPascalCase });
diff --git a/script/newConverter/writeConverter.js b/script/newConverter/writeConverter.js
@@ -11,19 +11,23 @@ module.exports.writeConverter = async ({ args, plugins, tslintPascalCase }) => {
           ]
         : ["", ""];
 
+    const body = args.eslint
+        ? `{${plugins}
+          rules: [
+              {${ruleArguments}
+                  ruleName: "${args.eslint}",
+              },
+          ],
+    }`
+        : `({})`;
+
     await fs.writeFile(
         `./src/converters/lintConfigs/rules/ruleConverters/${args.tslint}.ts`,
         `
-    import { RuleConverter } from "../ruleConverter";
+import { RuleConverter } from "../ruleConverter";
 
 export const convert${tslintPascalCase}: RuleConverter = (${functionArguments}) => {
-    return {${plugins}
-        rules: [
-            {${ruleArguments}
-                ruleName: "${args.eslint}",
-            },
-        ],
-    };
+    return ${body};
 };
 `.trimLeft(),
     );
diff --git a/script/newConverter/writeConverterTest.js b/script/newConverter/writeConverterTest.js
@@ -21,6 +21,16 @@ module.exports.writeConverterTest = async ({ args, tslintPascalCase, plugins })
         `
         : "";
 
+    const body = args.eslint
+        ? `
+            rules: [
+                {
+                    ruleName: "${args.eslint}",
+                },
+            ],
+        `
+        : "";
+
     await fs.writeFile(
         `./src/converters/lintConfigs/rules/ruleConverters/tests/${args.tslint}.test.ts`,
         `
@@ -32,13 +42,7 @@ describe(convert${tslintPascalCase}, () => {
             ruleArguments: [],
         });
 
-        expect(result).toEqual({${plugins.replace("\n", "\n    ")}
-            rules: [
-                {
-                    ruleName: "${args.eslint}",
-                },
-            ],
-        });
+        expect(result).toEqual({${plugins.replace("\n", "\n    ")}${body}});
     });${ruleArgumentsTest}
 });
 `.trimLeft(),
diff --git a/src/converters/comments/convertFileComments.test.ts b/src/converters/comments/convertFileComments.test.ts
@@ -55,6 +55,40 @@ describe("convertFileComments", () => {
         expect(dependencies.fileSystem.writeFile).not.toHaveBeenCalled();
     });
 
+    it("ignores comment contents when an input rule is obsolete", async () => {
+        // Arrange
+        const dependencies = {
+            ...createStubDependencies(`
+// tslint:disable
+export const a = true;
+
+// tslint:disable:obsolete
+export const b = true;
+`),
+            converters: new Map([["obsolete", () => ({})]]),
+        };
+
+        // Act
+        await convertFileComments(
+            dependencies,
+            stubFileName,
+            new Map<string, string[]>(),
+            new Map<string, string[]>(),
+        );
+
+        // Assert
+        expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
+            stubFileName,
+            `
+/* eslint-disable */
+export const a = true;
+
+/* eslint-disable */
+export const b = true;
+`,
+        );
+    });
+
     it("parses TSLint directives to their matching ESLint directives", async () => {
         // Arrange
         const dependencies = createStubDependencies(`
diff --git a/src/converters/comments/replaceFileComments.ts b/src/converters/comments/replaceFileComments.ts
@@ -29,7 +29,7 @@ export const replaceFileComments = (
             return undefined;
         }
 
-        const equivalents = converted.rules.map((conversion) => conversion.ruleName);
+        const equivalents = converted.rules?.map((conversion) => conversion.ruleName) ?? [];
 
         ruleCommentsCache.set(ruleName, equivalents);
 
diff --git a/src/converters/lintConfigs/configConversionResults.stubs.ts b/src/converters/lintConfigs/configConversionResults.stubs.ts
@@ -8,6 +8,7 @@ export const createEmptyConfigConversionResults = (
     extensionRules: new Map(),
     failed: [],
     missing: [],
+    obsolete: new Set(),
     plugins: new Set(),
     ruleEquivalents: new Map(),
     ...overrides,
diff --git a/src/converters/lintConfigs/convertLintConfig.ts b/src/converters/lintConfigs/convertLintConfig.ts
@@ -17,7 +17,7 @@ export type ConvertLintConfigDependencies = {
 
 /**
  * Root-level driver to convert a TSLint configuration to ESLint.
- * @see `/docs/Architecture/Linting.md` for documentation.
+ * @see `/docs/Architecture/Linters.md` for documentation.
  */
 export const convertLintConfig = async (
     dependencies: ConvertLintConfigDependencies,
diff --git a/src/converters/lintConfigs/reporting/reportConfigConversionResults.test.ts b/src/converters/lintConfigs/reporting/reportConfigConversionResults.test.ts
@@ -225,6 +225,44 @@ describe("reportConfigConversionResults", () => {
         );
     });
 
+    it("logs obsolete conversions when there is one obsolete conversion", async () => {
+        // Arrange
+        const logger = createStubLogger();
+        const conversionResults = createEmptyConfigConversionResults({
+            extends: basicExtends,
+            obsolete: new Set(["obsolete"]),
+        });
+
+        // Act
+        await reportConfigConversionResults({ logger }, ".eslintrc.js", conversionResults);
+
+        // Assert
+        expectEqualWrites(
+            logger.stdout.write,
+            `🦖 1 rule is obsolete and does not have an ESLint equivalent. 🦖`,
+            `  Check ${logger.debugFileName} for details.`,
+        );
+    });
+
+    it("logs obsolete conversions when there are multiple obsolete conversions", async () => {
+        // Arrange
+        const logger = createStubLogger();
+        const conversionResults = createEmptyConfigConversionResults({
+            extends: basicExtends,
+            obsolete: new Set(["obsolete-a", "obsolete-b"]),
+        });
+
+        // Act
+        await reportConfigConversionResults({ logger }, ".eslintrc.js", conversionResults);
+
+        // Assert
+        expectEqualWrites(
+            logger.stdout.write,
+            `🦖 2 rules are obsolete and do not have ESLint equivalents. 🦖`,
+            `  Check ${logger.debugFileName} for details.`,
+        );
+    });
+
     it("logs a Prettier recommendation when extends doesn't include eslint-config-prettier", async () => {
         // Arrange
         const logger = createStubLogger();
diff --git a/src/converters/lintConfigs/reporting/reportConfigConversionResults.ts b/src/converters/lintConfigs/reporting/reportConfigConversionResults.ts
@@ -5,6 +5,7 @@ import { Logger } from "../../../adapters/logger";
 import {
     logFailedConversions,
     logMissingConversionTarget,
+    logObsoleteRules,
     logSuccessfulConversions,
 } from "../../../reporting";
 import { ESLintRuleOptions, TSLintRuleOptions } from "../rules/types";
@@ -50,6 +51,10 @@ export const reportConfigConversionResults = async (
         );
     }
 
+    if (ruleConversionResults.obsolete.size !== 0) {
+        logObsoleteRules(Array.from(ruleConversionResults.obsolete), dependencies.logger);
+    }
+
     if (!ruleConversionResults.extends.join("").includes("prettier")) {
         logPrettierExtension(dependencies.logger);
     }
diff --git a/src/converters/lintConfigs/rules/convertRules.test.ts b/src/converters/lintConfigs/rules/convertRules.test.ts
@@ -71,6 +71,22 @@ describe("convertRules", () => {
         expect(failed).toEqual([conversionError]);
     });
 
+    it("marks a converted rule as obsolete when it has no output rules", () => {
+        // Arrange
+        const { tslintRule, converters, mergers } = setupConversionEnvironment();
+        converters.set(tslintRule.ruleName, () => ({}));
+
+        // Act
+        const { obsolete } = convertRules(
+            { ruleConverters: converters, ruleMergers: mergers },
+            { [tslintRule.ruleName]: tslintRule },
+            new Map<string, string[]>(),
+        );
+
+        // Assert
+        expect(Array.from(obsolete)).toEqual([tslintRule.ruleName]);
+    });
+
     it("marks a converted rule name as converted when a conversion has rules", () => {
         // Arrange
         const conversionResult = {
diff --git a/src/converters/lintConfigs/rules/convertRules.ts b/src/converters/lintConfigs/rules/convertRules.ts
@@ -21,12 +21,13 @@ export type RuleConversionResults = {
     failed: ErrorSummary[];
     missing: TSLintRuleOptions[];
     plugins: Set<string>;
+    obsolete: Set<string>;
     ruleEquivalents: Map<string, string[]>;
 };
 
 /**
  * Converts raw TSLint rules to their ESLint equivalents.
- * @see `/docs/Architecture/Linting.md` for documentation.
+ * @see `/docs/Architecture/Linters.md` for documentation.
  */
 export const convertRules = (
     dependencies: ConvertRulesDependencies,
@@ -36,6 +37,7 @@ export const convertRules = (
     const converted = new Map<string, ESLintRuleOptions>();
     const failed: ConversionError[] = [];
     const missing: TSLintRuleOptions[] = [];
+    const obsolete = new Set<string>();
     const plugins = new Set<string>();
 
     if (rawTslintRules !== undefined) {
@@ -48,7 +50,7 @@ export const convertRules = (
             // 2. The appropriate converter is run for the rule.
             const conversion = convertRule(tslintRule, dependencies.ruleConverters);
 
-            // 3. If the rule is missing or the conversion failed, this is marked.
+            // 3. If the rule is missing or obsolete, or the conversion failed, this is marked.
             if (conversion === undefined) {
                 if (tslintRule.ruleSeverity !== "off") {
                     missing.push(tslintRule);
@@ -62,6 +64,11 @@ export const convertRules = (
                 continue;
             }
 
+            if (!conversion.rules) {
+                obsolete.add(tslintRule.ruleName);
+                continue;
+            }
+
             const equivalents = new Set<string>();
 
             // 4. For each output rule equivalent given by the conversion:
@@ -119,5 +126,5 @@ export const convertRules = (
         }
     }
 
-    return { converted, failed, missing, plugins, ruleEquivalents };
+    return { converted, failed, missing, obsolete, plugins, ruleEquivalents };
 };
diff --git a/src/converters/lintConfigs/rules/ruleConverter.stubs.ts b/src/converters/lintConfigs/rules/ruleConverter.stubs.ts
@@ -1,11 +1,11 @@
 import { ConversionError } from "../../../errors/conversionError";
 
-export const createStubConverter = (result: ConversionError | string[]) => {
+export const createStubConverter = (result?: ConversionError | string[]) => {
     return () => {
         return result instanceof ConversionError
             ? result
             : {
-                  rules: result.map((ruleName) => ({ ruleName })),
+                  rules: result?.map((ruleName) => ({ ruleName })),
               };
     };
 };
diff --git a/src/converters/lintConfigs/rules/ruleConverter.ts b/src/converters/lintConfigs/rules/ruleConverter.ts
@@ -28,9 +28,9 @@ export type ConversionResult = {
     plugins?: string[];
 
     /**
-     * At least one equivalent ESLint rule and options.
+     * At least one equivalent ESLint rule and options, if not obsolete.
      */
-    rules: ConvertedRuleChanges[];
+    rules?: ConvertedRuleChanges[];
 };
 
 /**
diff --git a/src/converters/lintConfigs/rules/ruleConverters.ts b/src/converters/lintConfigs/rules/ruleConverters.ts
@@ -134,6 +134,7 @@ import { convertFileNameCasing } from "./ruleConverters/file-name-casing";
 import { convertForin } from "./ruleConverters/forin";
 import { convertFunctionConstructor } from "./ruleConverters/function-constructor";
 import { convertImportBlacklist } from "./ruleConverters/import-blacklist";
+import { convertImportDestructuringSpacing } from "./ruleConverters/import-destructuring-spacing";
 import { convertIncrementDecrement } from "./ruleConverters/increment-decrement";
 import { convertIndent } from "./ruleConverters/indent";
 import { convertInterfaceName } from "./ruleConverters/interface-name";
@@ -244,6 +245,7 @@ import { convertPreferConditionalExpression } from "./ruleConverters/prefer-cond
 import { convertPreferConst } from "./ruleConverters/prefer-const";
 import { convertPreferForOf } from "./ruleConverters/prefer-for-of";
 import { convertPreferFunctionOverMethod } from "./ruleConverters/prefer-function-over-method";
+import { convertPreferInlineDecorator } from "./ruleConverters/prefer-inline-decorator";
 import { convertPreferObjectSpread } from "./ruleConverters/prefer-object-spread";
 import { convertPreferReadonly } from "./ruleConverters/prefer-readonly";
 import { convertPreferSwitch } from "./ruleConverters/prefer-switch";
@@ -309,6 +311,7 @@ export const ruleConverters = new Map([
     ["forin", convertForin],
     ["function-constructor", convertFunctionConstructor],
     ["import-blacklist", convertImportBlacklist],
+    ["import-destructuring-spacing", convertImportDestructuringSpacing],
     ["increment-decrement", convertIncrementDecrement],
     ["indent", convertIndent],
     ["interface-name", convertInterfaceName],
@@ -478,6 +481,7 @@ export const ruleConverters = new Map([
     ["prefer-for-of", convertPreferForOf],
     ["prefer-function-over-method", convertPreferFunctionOverMethod],
     ["prefer-immediate-return", convertPreferImmediateReturn],
+    ["prefer-inline-decorator", convertPreferInlineDecorator],
     ["prefer-object-spread", convertPreferObjectSpread],
     ["prefer-on-push-component-change-detection", convertPreferOnPushComponentChangeDetection],
     ["prefer-output-readonly", convertPreferOutputReadonly],
diff --git a/src/converters/lintConfigs/rules/ruleConverters/import-destructuring-spacing.ts b/src/converters/lintConfigs/rules/ruleConverters/import-destructuring-spacing.ts
@@ -0,0 +1,5 @@
+import { RuleConverter } from "../ruleConverter";
+
+export const convertImportDestructuringSpacing: RuleConverter = () => {
+    return {};
+};
diff --git a/src/converters/lintConfigs/rules/ruleConverters/prefer-inline-decorator.ts b/src/converters/lintConfigs/rules/ruleConverters/prefer-inline-decorator.ts
@@ -0,0 +1,5 @@
+import { RuleConverter } from "../ruleConverter";
+
+export const convertPreferInlineDecorator: RuleConverter = () => {
+    return {};
+};
diff --git a/src/converters/lintConfigs/rules/ruleConverters/tests/import-destructuring-spacing.test.ts b/src/converters/lintConfigs/rules/ruleConverters/tests/import-destructuring-spacing.test.ts
diff --git a/src/converters/lintConfigs/rules/ruleConverters/tests/prefer-inline-decorator.test.ts b/src/converters/lintConfigs/rules/ruleConverters/tests/prefer-inline-decorator.test.ts
diff --git a/src/reporting.ts b/src/reporting.ts

Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ export const replaceFileComments = (`
`29`	`29`	`return undefined;`
`30`	`30`	`}`
`31`	`31`
`32`		`- const equivalents = converted.rules.map((conversion) => conversion.ruleName);`
	`32`	`+ const equivalents = converted.rules?.map((conversion) => conversion.ruleName) ?? [];`
`33`	`33`
`34`	`34`	`ruleCommentsCache.set(ruleName, equivalents);`
`35`	`35`