diff --git a/docs/Architecture/Linters.md b/docs/Architecture/Linters.md index 701c736a..17a08c52 100644 --- a/docs/Architecture/Linters.md +++ b/docs/Architecture/Linters.md @@ -23,6 +23,8 @@ Those are run by `src/converters/lintConfigs/rules/convertRules.ts`, which takes * 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. * If this is the first time the output ESLint rule is seen, it's directly marked as converted. + * Notices are merged and deduplicated. + * If the existing output has the same arguments as the new output, merge lookups are skipped. * If not, a rule merger is run to combine it with its existing output settings. ### Rule Converters @@ -52,6 +54,8 @@ 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. +> A merger does not need to be created if the rule does not accept any configuration or all converters output exactly the same configuration. + ## Package Summaries ESLint configurations are summarized based on extended ESLint and TSLint presets. diff --git a/src/converters/lintConfigs/rules/convertRules.test.ts b/src/converters/lintConfigs/rules/convertRules.test.ts index 64236f73..999e293b 100644 --- a/src/converters/lintConfigs/rules/convertRules.test.ts +++ b/src/converters/lintConfigs/rules/convertRules.test.ts @@ -107,41 +107,98 @@ describe("convertRules", () => { expect(ruleEquivalents).toEqual(new Map([["tslint-rule-a", ["eslint-rule-a"]]])); }); - it("reports a failure when two outputs exist for a converted rule without a merger", () => { - // Arrange - const conversionResult = { - rules: [ - { - ruleName: "eslint-rule-a", - }, - { - ruleName: "eslint-rule-a", - }, - ], - }; - const { tslintRule, converters, mergers } = setupConversionEnvironment({ - conversionResult, - }); - - // Act - const { failed } = convertRules( - { ruleConverters: converters, ruleMergers: mergers }, - { [tslintRule.ruleName]: tslintRule }, - new Map(), - ); - - // Assert - expect(failed).toEqual([ConversionError.forMerger("eslint-rule-a")]); - }); - - it("merges rule arguments when two outputs exist for a converted rule with a merger", () => { + test.each([[undefined], [[5, "allow-something", { max: 50 }]]])( + "runs without an argument merger when both rules have the same arguments", + (ruleArguments) => { + // Arrange + const conversionResult = { + rules: [ + { + ruleArguments, + ruleName: "eslint-rule-a", + }, + { + ruleArguments, + ruleName: "eslint-rule-a", + }, + ], + }; + const { tslintRule, converters, mergers } = setupConversionEnvironment({ + conversionResult, + }); + + // Act + const { converted, failed } = convertRules( + { ruleConverters: converters, ruleMergers: mergers }, + { [tslintRule.ruleName]: tslintRule }, + new Map(), + ); + + // Assert + expect(converted).toEqual( + new Map([ + [ + "eslint-rule-a", + { + ruleArguments, + ruleName: "eslint-rule-a", + ruleSeverity: "error", + notices: [], + }, + ], + ]), + ); + expect(failed).toEqual([]); + }, + ); + + test.each([ + [[[0]], [[1]]], + [[[""]], [["allow-something"]]], + [[[{ max: 0 }]], [[{ max: 50 }]]], + [[[0, "", { max: 0 }]], [[5, "allow-something", { max: 50 }]]], + ])( + "reports a failure when two outputs with different arguments exist for a converted rule without a merger", + ([existingArguments, newArguments]) => { + // Arrange + const conversionResult = { + rules: [ + { + ruleArguments: existingArguments, + ruleName: "eslint-rule-a", + }, + { + ruleArguments: newArguments, + ruleName: "eslint-rule-a", + }, + ], + }; + const { tslintRule, converters, mergers } = setupConversionEnvironment({ + conversionResult, + }); + + // Act + const { failed } = convertRules( + { ruleConverters: converters, ruleMergers: mergers }, + { [tslintRule.ruleName]: tslintRule }, + new Map(), + ); + + // Assert + expect(failed).toEqual([ConversionError.forMerger("eslint-rule-a")]); + }, + ); + + it("merges rule arguments when two outputs with different arguments exist for a converted rule with a merger", () => { // Arrange const conversionResult = { rules: [ { + ruleArguments: [0], ruleName: "eslint-rule-a", }, { + ruleArguments: [1], ruleName: "eslint-rule-a", }, ], @@ -201,7 +258,7 @@ describe("convertRules", () => { ); // Assert - expect(converted).toEqual( + expect([ new Map([ [ "eslint-rule-a", @@ -213,7 +270,17 @@ describe("convertRules", () => { }, ], ]), - ); + new Map([ + [ + "eslint-rule-a", + { + ruleName: "eslint-rule-a", + ruleSeverity: "error", + notices: ["notice-1", "notice-2"], + }, + ], + ]), + ]).toContainEqual(converted); }); it("merges undefined notices", () => { @@ -243,7 +310,7 @@ describe("convertRules", () => { ); // Assert - expect(converted).toEqual( + expect([ new Map([ [ "eslint-rule-a", @@ -255,7 +322,17 @@ describe("convertRules", () => { }, ], ]), - ); + new Map([ + [ + "eslint-rule-a", + { + ruleName: "eslint-rule-a", + ruleSeverity: "error", + notices: [], + }, + ], + ]), + ]).toContainEqual(converted); }); it("marks a new plugin when a conversion has a new plugin", () => { diff --git a/src/converters/lintConfigs/rules/convertRules.ts b/src/converters/lintConfigs/rules/convertRules.ts index f60ce38b..8b230f04 100644 --- a/src/converters/lintConfigs/rules/convertRules.ts +++ b/src/converters/lintConfigs/rules/convertRules.ts @@ -1,3 +1,5 @@ +import { isEqual } from "lodash"; + import { ConversionError } from "../../../errors/conversionError"; import { ErrorSummary } from "../../../errors/errorSummary"; import { TSLintConfigurationRules } from "../../../input/findTSLintConfiguration"; @@ -7,7 +9,7 @@ import { formatRawTslintRule } from "./formats/formatRawTslintRule"; import { RuleMerger } from "./ruleMerger"; import { RuleConverter } from "./ruleConverter"; import { TSLintRuleOptions, ESLintRuleOptions } from "./types"; -import { Entries } from "../../../utils"; +import { Entries, uniqueFromSources } from "../../../utils"; export type ConvertRulesDependencies = { ruleConverters: Map; @@ -78,21 +80,29 @@ export const convertRules = ( continue; } - // 4d. If not, a rule merger is run to combine it with its existing output settings. + // 4d. Notices are merged and deduplicated. + existingConversion.notices = uniqueFromSources( + existingConversion.notices, + newConversion.notices, + ); + converted.set(changes.ruleName, existingConversion); + + // 4e. If the existing output has the same arguments as the new output, merge lookups are skipped. + if (isEqual(existingConversion.ruleArguments, newConversion.ruleArguments)) { + continue; + } + + // 4f. If not, a rule merger is run to combine it with its existing output settings. const merger = dependencies.ruleMergers.get(changes.ruleName); if (merger === undefined) { failed.push(ConversionError.forMerger(changes.ruleName)); } else { - const existingNotices = existingConversion.notices ?? []; - const newNotices = newConversion.notices ?? []; - converted.set(changes.ruleName, { ...existingConversion, ruleArguments: merger( existingConversion.ruleArguments, newConversion.ruleArguments, ), - notices: Array.from(new Set([...existingNotices, ...newNotices])), }); } } diff --git a/src/converters/lintConfigs/rules/ruleMergers.ts b/src/converters/lintConfigs/rules/ruleMergers.ts index 66343a16..92c957ba 100644 --- a/src/converters/lintConfigs/rules/ruleMergers.ts +++ b/src/converters/lintConfigs/rules/ruleMergers.ts @@ -2,7 +2,6 @@ import { mergeBanTypes } from "./ruleMergers/ban-types"; import { mergeConsistentTypeAssertions } from "./ruleMergers/consistent-type-assertions"; import { mergeIndent } from "./ruleMergers/indent"; import { mergeNamingConvention } from "./ruleMergers/naming-convention"; -import { mergeNoCaller } from "./ruleMergers/no-caller"; import { mergeNoEval } from "./ruleMergers/no-eval"; import { mergeNoMemberDelimiterStyle } from "./ruleMergers/member-delimiter-style"; import { mergeNoUnnecessaryTypeAssertion } from "./ruleMergers/no-unnecessary-type-assertion"; @@ -16,6 +15,5 @@ export const ruleMergers = new Map([ ["@typescript-eslint/naming-convention", mergeNamingConvention], ["@typescript-eslint/no-unnecessary-type-assertion", mergeNoUnnecessaryTypeAssertion], ["@typescript-eslint/triple-slash-reference", mergeTripleSlashReference], - ["no-caller", mergeNoCaller], ["no-eval", mergeNoEval], ]); diff --git a/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/jsx-no-bind.ts b/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/jsx-no-bind.ts deleted file mode 100644 index de804928..00000000 --- a/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/jsx-no-bind.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { RuleMerger } from "../../ruleMerger"; - -export const mergeJsxNoBind: RuleMerger = () => { - return []; // jsx-no-bind rule does not accept any options -}; diff --git a/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/tests/jsx-no-bind.test.ts b/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/tests/jsx-no-bind.test.ts deleted file mode 100644 index 5988a9ed..00000000 --- a/src/converters/lintConfigs/rules/ruleMergers/eslint-plugin-react/tests/jsx-no-bind.test.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { mergeJsxNoBind } from "../jsx-no-bind"; - -describe(mergeJsxNoBind, () => { - test("neither options existing", () => { - const result = mergeJsxNoBind(undefined, undefined); - - expect(result).toEqual([]); - }); -}); diff --git a/src/converters/lintConfigs/rules/ruleMergers/no-caller.ts b/src/converters/lintConfigs/rules/ruleMergers/no-caller.ts deleted file mode 100644 index 687444e8..00000000 --- a/src/converters/lintConfigs/rules/ruleMergers/no-caller.ts +++ /dev/null @@ -1,6 +0,0 @@ -import { RuleMerger } from "../ruleMerger"; - -export const mergeNoCaller: RuleMerger = () => { - // no-caller rule does not accept any options - return []; -}; diff --git a/src/converters/lintConfigs/rules/ruleMergers/tests/no-caller.test.ts b/src/converters/lintConfigs/rules/ruleMergers/tests/no-caller.test.ts deleted file mode 100644 index adee30e5..00000000 --- a/src/converters/lintConfigs/rules/ruleMergers/tests/no-caller.test.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { mergeNoCaller } from "../no-caller"; - -describe(mergeNoCaller, () => { - test("neither options existing", () => { - const result = mergeNoCaller(undefined, undefined); - - expect(result).toEqual([]); - }); -});