Factored config rule conversions into comment conversions (#712)

Co-authored-by: KingDarBoja <[email protected]>

Author: Josh Goldberg

docs/Architecture.md

@@ -17,7 +17,7 @@ Within `src/conversion/convertLintConfig.ts`, the following steps occur:
     - 3a. If no output rules conflict with `eslint-config-prettier`, it's added in
     - 3b. Any ESLint rules that are configured the same as an extended preset are trimmed
 4. The summarized configuration is written to the output config file
-5. Files to transform comments in have source text rewritten using the same rule conversion logic
+5. Files to transform comments in have source text rewritten using the cached rule conversion results
 6. A summary of the results is printed to the user's console
 
 ### Conversion Results
@@ -51,6 +51,17 @@ These are located in `src/rules/mergers/`, and keyed under their names by the ma
 
 For example, `@typescript-eslint/ban-types` spreads both arguments' `types` members into one large `types` object.
 
+## Comment Conversion
+
+Comments are converted after rule conversion by `src/comments/convertComments.ts`.
+Source files are parsed into TypeScript files by `src/comments/parseFileComments.ts`, which then extracts their comment nodes.
+Those comments are parsed for TSLint rule disable or enable comments.
+
+Comments that match will be rewritten in their their file to their new ESLint rule equivalent in `src/comments/replaceFileComments.ts`, as determined by:
+
+1. First, if the `ruleEquivalents` cache received from configuration convertion has the TSLint rule's ESLint equivalents listed, those are used.
+2. Failing that, a comment-specific `ruleCommentsCache` is populated with rules converted ad-hoc with no arguments.
+
 ## Editor Configuration Conversion
 
 Editor lint configurations are converted by `src/editorSettings/convertEditorSettings.ts`.

src/comments/convertComments.test.ts

@@ -18,7 +18,7 @@ describe("convertComments", () => {
         const dependencies = createStubDependencies();
 
         // Act
-        const result = await convertComments(dependencies, undefined);
+        const result = await convertComments(dependencies, undefined, new Map());
 
         // Assert
         expect(result).toEqual({
@@ -35,7 +35,7 @@ describe("convertComments", () => {
         });
 
         // Act
-        const result = await convertComments(dependencies, true);
+        const result = await convertComments(dependencies, true, new Map());
 
         // Assert
         expect(result).toEqual({
@@ -52,7 +52,9 @@ describe("convertComments", () => {
         });
 
         // Act
-        const result = await convertComments(dependencies, ["*.ts"]);
+        const result = await convertComments(dependencies, true, new Map(), {
+            include: ["src/*.ts"],
+        });
 
         // Assert
         expect(result).toEqual({
@@ -71,7 +73,7 @@ describe("convertComments", () => {
         });
 
         // Act
-        const result = await convertComments(dependencies, []);
+        const result = await convertComments(dependencies, [], new Map());
 
         // Assert
         expect(result).toEqual({
@@ -91,7 +93,7 @@ describe("convertComments", () => {
         });
 
         // Act
-        const result = await convertComments(dependencies, []);
+        const result = await convertComments(dependencies, ["*.ts"], new Map());
 
         // Assert
         expect(result).toEqual({
@@ -108,7 +110,7 @@ describe("convertComments", () => {
         });
 
         // Act
-        const result = await convertComments(dependencies, ["*.ts"]);
+        const result = await convertComments(dependencies, ["*.ts"], new Map());
 
         // Assert
         expect(result).toEqual({
@@ -122,7 +124,7 @@ describe("convertComments", () => {
         const dependencies = createStubDependencies();
 
         // Act
-        const result = await convertComments(dependencies, ["*.ts"]);
+        const result = await convertComments(dependencies, ["*.ts"], new Map());
 
         // Assert
         expect(result).toEqual({

src/comments/convertComments.ts

@@ -17,6 +17,7 @@ export type ConvertCommentsDependencies = {
 export const convertComments = async (
     dependencies: ConvertCommentsDependencies,
     filePathGlobs: true | string | string[] | undefined,
+    ruleEquivalents: Map<string, string[]>,
     typescriptConfiguration?: TypeScriptConfiguration,
 ): Promise<ResultWithDataStatus<string[] | undefined>> => {
     if (filePathGlobs === undefined) {
@@ -76,11 +77,11 @@ export const convertComments = async (
         };
     }
 
-    const ruleConversionCache = new Map<string, string | undefined>();
+    const ruleCommentsCache = new Map<string, string[]>();
     const fileFailures = (
         await Promise.all(
             uniqueGlobbedFilePaths.map(async (filePath) =>
-                dependencies.convertFileComments(filePath, ruleConversionCache),
+                dependencies.convertFileComments(filePath, ruleCommentsCache, ruleEquivalents),
             ),
         )
     ).filter(isError);

src/comments/convertFileComments.test.ts

@@ -26,7 +26,7 @@ describe("convertFileComments", () => {
         const dependencies = createStubDependencies(readFileError);
 
         // Act
-        const result = await convertFileComments(dependencies, stubFileName, new Map());
+        const result = await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(result).toBe(readFileError);
@@ -39,7 +39,7 @@ describe("convertFileComments", () => {
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map());
+        await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(dependencies.fileSystem.writeFile).not.toHaveBeenCalled();
@@ -70,7 +70,7 @@ export const g = true;
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map());
+        await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
@@ -110,7 +110,7 @@ export const b = true;
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map());
+        await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
@@ -125,15 +125,45 @@ export const b = true;
         );
     });
 
-    it("re-uses a rule conversion from cache when it was already converted", async () => {
+    it("re-uses a rule conversion from conversion cache when it was already converted", async () => {
         // Arrange
         const dependencies = createStubDependencies(`
 /* tslint:disable:ts-a */
 export const a = true;
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map([["ts-a", "es-cached"]]));
+        await convertFileComments(
+            dependencies,
+            stubFileName,
+            new Map(),
+            new Map([["ts-a", ["es-cached"]]]),
+        );
+
+        // Assert
+        expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
+            stubFileName,
+            `
+/* eslint-disable es-cached */
+export const a = true;
+`,
+        );
+    });
+
+    it("re-uses a rule conversion from comments cache when it was already converted", async () => {
+        // Arrange
+        const dependencies = createStubDependencies(`
+/* tslint:disable:ts-a */
+export const a = true;
+`);
+
+        // Act
+        await convertFileComments(
+            dependencies,
+            stubFileName,
+            new Map([["ts-a", ["es-cached"]]]),
+            new Map(),
+        );
 
         // Assert
         expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
@@ -153,7 +183,7 @@ export const a = true;
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map());
+        await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(
@@ -173,7 +203,7 @@ export const a = true;
 `);
 
         // Act
-        await convertFileComments(dependencies, stubFileName, new Map());
+        await convertFileComments(dependencies, stubFileName, new Map(), new Map());
 
         // Assert
         expect(dependencies.fileSystem.writeFile).toHaveBeenCalledWith(

src/comments/convertFileComments.ts

@@ -11,7 +11,8 @@ export type ConvertFileCommentsDependencies = {
 export const convertFileComments = async (
     dependencies: ConvertFileCommentsDependencies,
     filePath: string,
-    ruleConversionCache: Map<string, string | undefined>,
+    ruleCommentsCache: Map<string, string[]>,
+    ruleEquivalents: Map<string, string[]>,
 ) => {
     const fileContent = await dependencies.fileSystem.readFile(filePath);
     if (fileContent instanceof Error) {
@@ -23,7 +24,8 @@ export const convertFileComments = async (
         fileContent,
         comments,
         dependencies.converters,
-        ruleConversionCache,
+        ruleCommentsCache,
+        ruleEquivalents,
     );
 
     return fileContent === newFileContent

src/comments/replaceFileComments.ts

@@ -9,23 +9,31 @@ export const replaceFileComments = (
     content: string,
     comments: FileComment[],
     converters: Map<string, RuleConverter>,
-    ruleConversionCache: Map<string, string | undefined>,
+    ruleCommentsCache: Map<string, string[]>,
+    ruleEquivalents: Map<string, string[]>,
 ) => {
     const getNewRuleLists = (ruleName: string) => {
-        if (ruleConversionCache.has(ruleName)) {
-            return ruleConversionCache.get(ruleName);
+        const cached = ruleEquivalents.get(ruleName) ?? ruleCommentsCache.get(ruleName);
+        if (cached !== undefined) {
+            return cached;
         }
 
         const converter = converters.get(ruleName);
         if (converter === undefined) {
-            ruleConversionCache.set(ruleName, undefined);
+            ruleCommentsCache.set(ruleName, []);
             return undefined;
         }
 
         const converted = converter({ ruleArguments: [] });
-        return converted instanceof ConversionError
-            ? undefined
-            : converted.rules.map((conversion) => conversion.ruleName).join(", ");
+        if (converted instanceof ConversionError) {
+            return undefined;
+        }
+
+        const equivalents = converted.rules.map((conversion) => conversion.ruleName);
+
+        ruleCommentsCache.set(ruleName, equivalents);
+
+        return equivalents.join(", ");
     };
 
     for (const comment of [...comments].reverse()) {

src/conversion/conversionResults.stubs.ts

@@ -10,6 +10,7 @@ export const createEmptyConversionResults = (
     failed: [],
     missing: [],
     plugins: new Set(),
+    ruleEquivalents: new Map(),
     ...overrides,
 });
 

src/conversion/convertLintConfig.test.ts

@@ -1,4 +1,5 @@
 import { ResultStatus, FailedResult } from "../types";
+import { createEmptyConversionResults } from "./conversionResults.stubs";
 import { convertLintConfig, ConvertLintConfigDependencies } from "./convertLintConfig";
 
 const stubSettings = {
@@ -7,28 +8,27 @@ const stubSettings = {
 
 const createStubDependencies = (
     overrides: Partial<ConvertLintConfigDependencies> = {},
-): ConvertLintConfigDependencies => ({
-    convertComments: jest.fn(),
-    convertRules: jest.fn(),
-    findOriginalConfigurations: jest.fn().mockResolvedValue({
-        data: createStubOriginalConfigurationsData(),
-        status: ResultStatus.Succeeded,
-    }),
-    reportCommentResults: jest.fn(),
-    reportConversionResults: jest.fn(),
-    summarizePackageRules: async (_configurations, data) => ({
-        ...data,
-        converted: new Map(),
-        extends: [],
-        extensionRules: new Map(),
-        failed: [],
-        missing: [],
-        plugins: new Set(),
-    }),
-    logMissingPackages: jest.fn().mockReturnValue(Promise.resolve()),
-    writeConversionResults: jest.fn().mockReturnValue(Promise.resolve()),
-    ...overrides,
-});
+): ConvertLintConfigDependencies => {
+    const ruleConversionResults = createEmptyConversionResults();
+
+    return {
+        convertComments: jest.fn(),
+        convertRules: jest.fn().mockReturnValue(ruleConversionResults),
+        findOriginalConfigurations: jest.fn().mockResolvedValue({
+            data: createStubOriginalConfigurationsData(),
+            status: ResultStatus.Succeeded,
+        }),
+        reportCommentResults: jest.fn(),
+        reportConversionResults: jest.fn(),
+        summarizePackageRules: async (_configurations, data) => ({
+            ...ruleConversionResults,
+            ...data,
+        }),
+        logMissingPackages: jest.fn().mockReturnValue(Promise.resolve()),
+        writeConversionResults: jest.fn().mockReturnValue(Promise.resolve()),
+        ...overrides,
+    };
+};
 
 const createStubOriginalConfigurationsData = () => ({
     tslint: {

src/conversion/convertLintConfig.ts

@@ -60,9 +60,10 @@ export const convertLintConfig = async (
         };
     }
 
-    // 5. Files to transform comments in have source text rewritten using the same rule conversion logic
+    // 5. Files to transform comments in have source text rewritten using the cached rule conversion results
     const commentsResult = await dependencies.convertComments(
         settings.comments,
+        ruleConversionResults.ruleEquivalents,
         originalConfigurations.data.typescript,
     );
 

src/rules/convertRules.ts

@@ -18,6 +18,7 @@ export type RuleConversionResults = {
     failed: ErrorSummary[];
     missing: TSLintRuleOptions[];
     plugins: Set<string>;
+    ruleEquivalents: Map<string, string[]>;
 };
 
 export const convertRules = (
@@ -28,6 +29,7 @@ export const convertRules = (
     const failed: ConversionError[] = [];
     const missing: TSLintRuleOptions[] = [];
     const plugins = new Set<string>();
+    const ruleEquivalents = new Map<string, string[]>();
 
     if (rawTslintRules !== undefined) {
         for (const [ruleName, value] of Object.entries(rawTslintRules)) {
@@ -47,7 +49,11 @@ export const convertRules = (
                 continue;
             }
 
+            const equivalents = new Set<string>();
+
             for (const changes of conversion.rules) {
+                equivalents.add(changes.ruleName);
+
                 const existingConversion = converted.get(changes.ruleName);
                 const newConversion = {
                     ...changes,
@@ -82,7 +88,9 @@ export const convertRules = (
                     plugins.add(newPlugin);
                 }
             }
+
+            ruleEquivalents.set(tslintRule.ruleName, Array.from(equivalents));
         }
     }
-    return { converted, failed, missing, plugins };
+    return { converted, failed, missing, plugins, ruleEquivalents };
 };