From 06b6358daee6a7e220f7162f50be8174a154d176 Mon Sep 17 00:00:00 2001 From: Emily Giurleo Date: Sun, 11 Aug 2019 21:44:36 -0400 Subject: [PATCH 1/3] merge and deduplicate rule notices --- src/rules/convertRules.test.ts | 46 ++++++++++++++++++++++++++++++++++ src/rules/convertRules.ts | 3 ++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/rules/convertRules.test.ts b/src/rules/convertRules.test.ts index 20aa66b90..eed3bbd29 100644 --- a/src/rules/convertRules.test.ts +++ b/src/rules/convertRules.test.ts @@ -95,6 +95,7 @@ describe("convertRules", () => { { ruleName: "eslint-rule-a", ruleSeverity: "error", + notices: [], }, ], ]), @@ -183,6 +184,51 @@ describe("convertRules", () => { ); }); + it("merges and deduplicates rule notices", () => { + // Arrange + const tslintRule: TSLintRuleOptions = { + ruleArguments: [], + ruleName: "tslint-rule-a", + ruleSeverity: "error", + }; + const conversionResult = { + rules: [ + { + ruleName: "eslint-rule-a", + notices: ["notice-1", "notice-2"], + }, + { + ruleName: "eslint-rule-a", + notices: ["notice-1"], + }, + ], + }; + const mergedArguments = [{ merged: true }]; + const converters = new Map([[tslintRule.ruleName, () => conversionResult]]); + const mergers = new Map([[conversionResult.rules[0].ruleName, () => mergedArguments]]); + + // Act + const { converted } = convertRules( + { converters, mergers }, + { [tslintRule.ruleName]: tslintRule }, + ); + + // Assert + expect(converted).toEqual( + new Map([ + [ + "eslint-rule-a", + { + ruleArguments: mergedArguments, + ruleName: "eslint-rule-a", + ruleSeverity: "error", + notices: ["notice-1", "notice-2"], + }, + ], + ]), + ); + }); + it("marks a new plugin when a conversion has a new plugin", () => { // Arrange const tslintRule: TSLintRuleOptions = { diff --git a/src/rules/convertRules.ts b/src/rules/convertRules.ts index 8090c5a11..87213d7a8 100644 --- a/src/rules/convertRules.ts +++ b/src/rules/convertRules.ts @@ -50,6 +50,7 @@ export const convertRules = ( const existingConversion = converted.get(changes.ruleName); const newConversion = { ...changes, + notices: changes.notices || [], ruleSeverity: convertTSLintRuleSeverity(tslintRule.ruleSeverity), }; @@ -78,7 +79,7 @@ export const convertRules = ( existingConversion.ruleArguments, newConversion.ruleArguments, ), - notices: [...existingNotices, ...newNotices], + notices: Array.from(new Set([...existingNotices, ...newNotices])), }); } } From abb5e87d057c75711cbcaf1734ce6e2ddfc3da69 Mon Sep 17 00:00:00 2001 From: Emily Giurleo Date: Sun, 11 Aug 2019 21:51:14 -0400 Subject: [PATCH 2/3] small refactor --- src/rules/convertRules.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/rules/convertRules.ts b/src/rules/convertRules.ts index 87213d7a8..a329d5c88 100644 --- a/src/rules/convertRules.ts +++ b/src/rules/convertRules.ts @@ -48,6 +48,7 @@ export const convertRules = ( for (const changes of conversion.rules) { const existingConversion = converted.get(changes.ruleName); + const newConversion = { ...changes, notices: changes.notices || [], @@ -57,6 +58,8 @@ export const convertRules = ( if (existingConversion === undefined) { converted.set(changes.ruleName, newConversion); continue; + } else { + existingConversion.notices = existingConversion.notices || []; } const merger = dependencies.mergers.get(changes.ruleName); @@ -70,16 +73,15 @@ export const convertRules = ( ), ); } 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])), + notices: Array.from( + new Set([...existingConversion.notices, ...newConversion.notices]), + ), }); } } From 27d225737c604cbcf2c7c1d18cb757f0f1db3e44 Mon Sep 17 00:00:00 2001 From: Emily Giurleo Date: Mon, 12 Aug 2019 20:42:06 -0400 Subject: [PATCH 3/3] make sure notices arent going to be undefined when theyre passed into sets --- src/rules/convertRules.test.ts | 46 +++++++++++++++++++++++++++++++++- src/rules/convertRules.ts | 11 +++----- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/rules/convertRules.test.ts b/src/rules/convertRules.test.ts index eed3bbd29..8ac1ae890 100644 --- a/src/rules/convertRules.test.ts +++ b/src/rules/convertRules.test.ts @@ -95,7 +95,6 @@ describe("convertRules", () => { { ruleName: "eslint-rule-a", ruleSeverity: "error", - notices: [], }, ], ]), @@ -229,6 +228,51 @@ describe("convertRules", () => { ); }); + it("merges undefined notices", () => { + // Arrange + const tslintRule: TSLintRuleOptions = { + ruleArguments: [], + ruleName: "tslint-rule-a", + ruleSeverity: "error", + }; + const conversionResult = { + rules: [ + { + ruleName: "eslint-rule-a", + notices: undefined, + }, + { + ruleName: "eslint-rule-a", + notices: undefined, + }, + ], + }; + const mergedArguments = [{ merged: true }]; + const converters = new Map([[tslintRule.ruleName, () => conversionResult]]); + const mergers = new Map([[conversionResult.rules[0].ruleName, () => mergedArguments]]); + + // Act + const { converted } = convertRules( + { converters, mergers }, + { [tslintRule.ruleName]: tslintRule }, + ); + + // Assert + expect(converted).toEqual( + new Map([ + [ + "eslint-rule-a", + { + ruleArguments: mergedArguments, + ruleName: "eslint-rule-a", + ruleSeverity: "error", + notices: [], + }, + ], + ]), + ); + }); + it("marks a new plugin when a conversion has a new plugin", () => { // Arrange const tslintRule: TSLintRuleOptions = { diff --git a/src/rules/convertRules.ts b/src/rules/convertRules.ts index a329d5c88..d28c42199 100644 --- a/src/rules/convertRules.ts +++ b/src/rules/convertRules.ts @@ -48,18 +48,14 @@ export const convertRules = ( for (const changes of conversion.rules) { const existingConversion = converted.get(changes.ruleName); - const newConversion = { ...changes, - notices: changes.notices || [], ruleSeverity: convertTSLintRuleSeverity(tslintRule.ruleSeverity), }; if (existingConversion === undefined) { converted.set(changes.ruleName, newConversion); continue; - } else { - existingConversion.notices = existingConversion.notices || []; } const merger = dependencies.mergers.get(changes.ruleName); @@ -73,15 +69,16 @@ export const convertRules = ( ), ); } 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([...existingConversion.notices, ...newConversion.notices]), - ), + notices: Array.from(new Set([...existingNotices, ...newNotices])), }); } }