Properly use ruleSeverity properties of rule converters during conversion

[hyperupcall]

PR Checklist

• Addresses an existing issue: fixes Conversion of TSLint semicolon rule seems incorrect? #1207
• That issue was marked as status: accepting prs

Overview

It seems that if ruleSeverity is specified within a rule of a converter, it is not properly applied when converting to the ESLint rules. The linked page mentions it specifically for semicolon, but it seems to be more general

For example, the following TSLint rules will both have a ruleSeverity of error since true is the first array element

{
    "rules": {
        "semicolon": [true, "always"],
        "no-variable-usage-before-declaration": [true]
    }
}

This translates directly to the following ESLint, even though we do { ruleName: "semi", ruleSeverity: "off" }, etc. in the converters

{
    "rules": {
        "no-use-before-define": "error",
        "semi": "error"
    }
}

The patches fix this by setting the converted ruleSeverity with the explicitly specified rule.ruleSeverity if it is truthy

The following show the full resulting ESLint configs before and after the code changes (using the aforementioned TSLint config)

Before:

.eslintrc.json

{
    "env": {
        "browser": true,
        "es6": true
    },
    "extends": [
        "prettier"
    ],
    "parser": "@typescript-eslint/parser",
    "parserOptions": {
        "project": "tsconfig.json",
        "sourceType": "module"
    },
    "plugins": [
        "@typescript-eslint"
    ],
    "root": true,
    "rules": {
        "@typescript-eslint/member-delimiter-style": [
            "error",
            {
                "multiline": {
                    "delimiter": "semi",
                    "requireLast": true
                },
                "singleline": {
                    "delimiter": "semi",
                    "requireLast": false
                }
            }
        ],
        "@typescript-eslint/no-use-before-define": [
            "error",
            {
                "variables": true
            }
        ],
        "@typescript-eslint/semi": [
            "error",
            "always"
        ],
        "no-use-before-define": "error",
        "semi": "error"
    }
}

After:

.eslintrc.json

{
    "env": {
        "browser": true,
        "es6": true
    },
    "extends": [
        "prettier"
    ],
    "parser": "@typescript-eslint/parser",
    "parserOptions": {
        "project": "tsconfig.json",
        "sourceType": "module"
    },
    "plugins": [
        "@typescript-eslint"
    ],
    "root": true,
    "rules": {
        "@typescript-eslint/member-delimiter-style": [
            "error",
            {
                "multiline": {
                    "delimiter": "semi",
                    "requireLast": true
                },
                "singleline": {
                    "delimiter": "semi",
                    "requireLast": false
                }
            }
        ],
        "@typescript-eslint/no-use-before-define": [
            "error",
            {
                "variables": true
            }
        ],
        "@typescript-eslint/semi": [
            "error",
            "always"
        ],
        "no-use-before-define": "off",
        "semi": "off"
    }
}

[JoshuaKGoldberg]

Looks great so far, thanks for starting this!

Just requesting changes on an additional test. Everything else is nitpicking 😄 .

[JoshuaKGoldberg]

Nit: the array type there is the ConvertedRuleChanges exported by ruleConverter.ts.

Suggested change

	const conversionResult: {
	rules: { ruleName: string; ruleSeverity: ESLintRuleSeverity }[];
	} = {
	const conversionResult: {
	rules: ConvertedRuleChanges[];
	} = {

Although, I think this would be a bit cleaner:

const rules: ConvertedRuleChanges[] = [
    {
        ruleName: "eslint-rule-a",
        ruleSeverity: "off",
    },
];

const { tslintRule, converters, mergers } = setupConversionEnvironment({
    ruleSeverity: "error",
    conversionResult: { rules },
    ruleToMerge: rules[0].ruleName,
});

What do you think?

[hyperupcall]

I'll incorporate those changes - agree that it looks cleaner! :)

[JoshuaKGoldberg]

Nit: the type arguments can be inferred here.

Suggested change

	new Map<string, string[]>(),
	new Map(),

Eventually we'll have a no-inferrable-type-arguments rule or some such in typescript-eslint... one day!

[JoshuaKGoldberg]

This test case looks good and we should include it. Great!

But I think we should also test for the case described in the issue: that there are two output rules from the converter, one turned off and one with the original rule severity. Could you please add a test case for that?

[hyperupcall]

For sure! I'll incorporate that new test case in my upcoming modifications ^w^

[hyperupcall]

Thank you for your feedback! I'll be pushing the new changes within the next day

EDIT: Done! I also edited the test name to make it sound a bit better

[JoshuaKGoldberg]

Changes look great, thanks as always @hyperupcall! ✨