Skip to content

Commit 7b0ebdf

Browse files
author
Josh Goldberg
authored
Improved error message and issue template for missing rule mergers (#145)
* Improved error message and issue template for missing rule mergers * Disab-I mean, fixed ESLint rule complaints
1 parent 861ac75 commit 7b0ebdf

16 files changed

+148
-88
lines changed

‎.eslintrc.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@ module.exports = {
1818
},
1919
plugins: ["@typescript-eslint"],
2020
rules: {
21-
"@typescript-eslint/consistent-type-definitions": ["error", "type"],
21+
"@typescript-eslint/consistent-type-definitions": 0,
2222
"@typescript-eslint/explicit-function-return-type": 0,
2323
"@typescript-eslint/generic-type-naming": 0,
2424
"@typescript-eslint/indent": 0,
25+
"@typescript-eslint/member-ordering": 0,
2526
"@typescript-eslint/no-explicit-any": 0,
2627
"@typescript-eslint/no-extra-parens": 0,
2728
"@typescript-eslint/no-magic-numbers": 0,

‎.github/ISSUE_TEMPLATE/missing_merger.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
name: Missing Merger
3+
about: Report a missing ESLint rule merger in tslint-to-eslint-config
4+
---
5+
6+
<!--
7+
👋 Hi, thanks for filing an issue on tslint-to-eslint-config! 💖
8+
Please fill out all fields below to ensure your issue is addressed.
9+
10+
If your issue doesn't provide enough info to fully explain or reproduce your bug, it will be closed. 😦
11+
-->
12+
13+
### 💥 Missing Merger
14+
15+
- `tslint-to-eslint-config` version: X.X.X
16+
- ESLint version: X.X.X
17+
18+
#### CLI Output
19+
20+
<!-- What did tslint-to-eslint-config print to the screen? -->
21+
22+
#### File Output
23+
24+
<!-- What did tslint-to-eslint-config print to tslint-to-eslint-config.log? -->
25+
26+
#### Reproduction
27+
28+
<!--
29+
Please paste a link to a repository, Gist, or other means of reproducing your error here.
30+
Note that `tslint-to-eslint-config` includes information from `package.json`, `tsconfig.json`, and other files, so just your TSLint configuration may not be enough.
31+
-->

‎src/errors/configurationError.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { EOL } from "os";
2+
3+
import { ConfigurationError } from "./configurationError";
4+
5+
describe(ConfigurationError, () => {
6+
describe("getSummary", () => {
7+
it("creates a summary for the error", () => {
8+
// Arrange
9+
const rawError = new Error("Oh no!");
10+
const complaint = "It broke";
11+
const error = new ConfigurationError(rawError, complaint);
12+
13+
// Act
14+
const summary = error.getSummary();
15+
16+
// Assert
17+
expect(summary).toEqual(`It broke: ${rawError.stack}${EOL}`);
18+
});
19+
});
20+
});

‎src/errors/configurationError.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1-
export class ConfigurationError {
2-
public constructor(public readonly error: Error, public readonly complaint: string) {}
1+
import { EOL } from "os";
2+
3+
import { ErrorSummary } from "./errorSummary";
4+
5+
export class ConfigurationError implements ErrorSummary {
6+
public constructor(private readonly error: Error, private readonly complaint: string) {}
7+
8+
public getSummary(): string {
9+
return `${this.complaint}: ${this.error.stack}${EOL}`;
10+
}
311
}

‎src/errors/conversionError.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { EOL } from "os";
2+
3+
import { ConversionError } from "./conversionError";
4+
import { TSLintRuleOptions } from "../rules/types";
5+
6+
describe(ConversionError, () => {
7+
describe("getSummary", () => {
8+
it("prints the error stack when created for an error", () => {
9+
// Arrange
10+
const error = new Error("Oh no!");
11+
const tslintRule: TSLintRuleOptions = {
12+
ruleArguments: [],
13+
ruleName: "rule-a",
14+
ruleSeverity: "warning",
15+
};
16+
const conversionError = ConversionError.forRuleError(error, tslintRule);
17+
18+
// Act
19+
const summary = conversionError.getSummary();
20+
21+
// Assert
22+
expect(summary).toEqual(
23+
`rule-a threw an error during conversion: ${error.stack}${EOL}`,
24+
);
25+
});
26+
27+
it("prints a merger complaint when created for a merger", () => {
28+
// Arrange
29+
const conversionError = ConversionError.forMerger("eslint-rule");
30+
31+
// Act
32+
const summary = conversionError.getSummary();
33+
34+
// Assert
35+
expect(summary).toEqual(
36+
`Error: multiple output eslint-rule ESLint rule options were generated, but tslint-to-eslint-config doesn't have "merger" logic to deal with this.${EOL}Please file an issue at https://github.com/typescript-eslint/tslint-to-eslint-config/issues/new?template=bug_report.md!`,
37+
);
38+
});
39+
});
40+
});

‎src/errors/conversionError.ts

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,27 @@
1+
import { EOL } from "os";
2+
13
import { TSLintRuleOptions } from "../rules/types";
4+
import { ErrorSummary } from "./errorSummary";
5+
6+
export class ConversionError implements ErrorSummary {
7+
private constructor(private readonly summary: string) {}
8+
9+
public static forMerger = (eslintRule: string) => {
10+
return new ConversionError(
11+
[
12+
`Error: multiple output ${eslintRule} ESLint rule options were generated, but tslint-to-eslint-config doesn't have "merger" logic to deal with this.`,
13+
`Please file an issue at https://github.com/typescript-eslint/tslint-to-eslint-config/issues/new?template=missing_merger.md 🙏`,
14+
].join(EOL),
15+
);
16+
};
17+
18+
public getSummary(): string {
19+
return this.summary;
20+
}
221

3-
export class ConversionError {
4-
public constructor(
5-
public readonly error: Error,
6-
public readonly tslintRule: TSLintRuleOptions,
7-
) {}
22+
public static forRuleError = (error: Error, tslintRule: TSLintRuleOptions) => {
23+
return new ConversionError(
24+
`${tslintRule.ruleName} threw an error during conversion: ${error.stack}${EOL}`,
25+
);
26+
};
827
}

‎src/errors/errorSummary.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export interface ErrorSummary {
2+
getSummary(): string;
3+
}

‎src/reporting/reportConversionResults.test.ts

Lines changed: 5 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
import { EOL } from "os";
2-
import { ESLintRuleOptions } from "../rules/types";
3-
import { reportConversionResults } from "./reportConversionResults";
2+
43
import { createStubLogger, expectEqualWrites } from "../adapters/logger.stubs";
54
import { createEmptyConversionResults } from "../conversion/conversionResults.stubs";
6-
import { ConversionError } from "../errors/conversionError";
7-
import { ConfigurationError } from "../errors/configurationError";
5+
import { ESLintRuleOptions } from "../rules/types";
6+
import { reportConversionResults } from "./reportConversionResults";
87

98
describe("reportConversionResults", () => {
109
it("logs a successful conversion without notices when there is one converted rule without notices", () => {
@@ -110,35 +109,10 @@ describe("reportConversionResults", () => {
110109
);
111110
});
112111

113-
it("logs a failed configuration when there is one failed configuration error", () => {
114-
// Arrange
115-
const conversionResults = createEmptyConversionResults({
116-
failed: [new ConfigurationError(new Error("and a one"), "some complaint")],
117-
});
118-
119-
const logger = createStubLogger();
120-
121-
// Act
122-
reportConversionResults({ logger }, conversionResults);
123-
124-
// Assert
125-
expectEqualWrites(
126-
logger.stderr.write,
127-
"💀 1 error thrown. 💀",
128-
`Check ${logger.debugFileName} for details.`,
129-
);
130-
});
131-
132112
it("logs a failed conversion when there is one failed conversion", () => {
133113
// Arrange
134114
const conversionResults = createEmptyConversionResults({
135-
failed: [
136-
new ConversionError(new Error("and a one"), {
137-
ruleArguments: ["a", "b"],
138-
ruleName: "tslint-rule-one",
139-
ruleSeverity: "error",
140-
}),
141-
],
115+
failed: [{ getSummary: () => "It broke." }],
142116
});
143117

144118
const logger = createStubLogger();
@@ -157,18 +131,7 @@ describe("reportConversionResults", () => {
157131
it("logs failed conversions when there are multiple failed conversions", () => {
158132
// Arrange
159133
const conversionResults = createEmptyConversionResults({
160-
failed: [
161-
new ConversionError(new Error("and a one"), {
162-
ruleArguments: ["a", "b"],
163-
ruleName: "tslint-rule-one",
164-
ruleSeverity: "error",
165-
}),
166-
new ConversionError(new Error("and a two"), {
167-
ruleArguments: ["c", "d"],
168-
ruleName: "tslint-rule-two",
169-
ruleSeverity: "warning",
170-
}),
171-
],
134+
failed: [{ getSummary: () => "It broke." }, { getSummary: () => "It really broke." }],
172135
});
173136

174137
const logger = createStubLogger();

‎src/reporting/reportConversionResults.ts

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ import chalk from "chalk";
22
import { EOL } from "os";
33

44
import { Logger } from "../adapters/logger";
5-
import { ConfigurationError } from "../errors/configurationError";
6-
import { ConversionError } from "../errors/conversionError";
5+
import { ErrorSummary } from "../errors/errorSummary";
76
import { RuleConversionResults } from "../rules/convertRules";
87
import { TSLintRuleOptions, ESLintRuleOptions } from "../rules/types";
98

@@ -43,21 +42,13 @@ const logSuccessfulConversions = (converted: Map<string, ESLintRuleOptions>, log
4342
logger.stdout.write(chalk.greenBright(` ✨${EOL}`));
4443
};
4544

46-
const logFailedConversions = (failed: (ConfigurationError | ConversionError)[], logger: Logger) => {
45+
const logFailedConversions = (failed: ErrorSummary[], logger: Logger) => {
4746
logger.stderr.write(`${chalk.redBright(`💀 ${failed.length}`)}`);
4847
logger.stderr.write(chalk.red(` error${failed.length === 1 ? "" : "s"}`));
4948
logger.stderr.write(chalk.red(" thrown."));
5049
logger.stderr.write(chalk.redBright(` 💀${EOL}`));
5150

52-
logger.info.write(
53-
failed
54-
.map(failed =>
55-
failed instanceof ConfigurationError
56-
? `${failed.complaint}: ${failed.error.stack}${EOL}`
57-
: `${failed.tslintRule.ruleName} threw an error during conversion: ${failed.error.stack}${EOL}`,
58-
)
59-
.join(""),
60-
);
51+
logger.info.write(failed.map(failed => failed.getSummary()).join(""));
6152

6253
logger.stderr.write(chalk.gray(`Check ${logger.debugFileName} for details.${EOL}`));
6354
};

‎src/rules/convertRule.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,6 @@ describe("convertRule", () => {
6868
const result = convertRule(tslintRule, converters);
6969

7070
// Assert
71-
expect(result).toEqual(new ConversionError(error, tslintRule));
71+
expect(result).toEqual(ConversionError.forRuleError(error, tslintRule));
7272
});
7373
});

‎src/rules/convertRule.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,6 @@ export const convertRule = (
1414
try {
1515
return converter(tslintRule);
1616
} catch (error) {
17-
return new ConversionError(error, tslintRule);
17+
return ConversionError.forRuleError(error, tslintRule);
1818
}
1919
};

‎src/rules/convertRules.test.ts

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ describe("convertRules", () => {
5050
ruleName: "tslint-rule-a",
5151
ruleSeverity: "error",
5252
};
53-
const conversionError = new ConversionError(new Error(), tslintRule);
53+
const conversionError = ConversionError.forRuleError(new Error(), tslintRule);
5454
const converters = new Map([[tslintRule.ruleName, () => conversionError]]);
5555
const mergers = new Map();
5656

@@ -128,16 +128,7 @@ describe("convertRules", () => {
128128
);
129129

130130
// Assert
131-
expect(failed).toEqual(
132-
jasmine.arrayContaining([
133-
jasmine.objectContaining({
134-
error: jasmine.objectContaining({
135-
message: `No merger for multiple output eslint-rule-a rule configurations.`,
136-
}),
137-
tslintRule,
138-
}),
139-
]),
140-
);
131+
expect(failed).toEqual([ConversionError.forMerger("eslint-rule-a")]);
141132
});
142133

143134
it("merges rule arguments two outputs exist for a converted rule with a merger", () => {

‎src/rules/convertRules.ts

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { ConfigurationError } from "../errors/configurationError";
21
import { ConversionError } from "../errors/conversionError";
2+
import { ErrorSummary } from "../errors/errorSummary";
33
import { TSLintConfigurationRules } from "../input/findTSLintConfiguration";
44
import { RuleConverter } from "./converter";
55
import { convertRule } from "./convertRule";
@@ -15,7 +15,7 @@ export type ConvertRulesDependencies = {
1515

1616
export type RuleConversionResults = {
1717
converted: Map<string, ESLintRuleOptions>;
18-
failed: (ConfigurationError | ConversionError)[];
18+
failed: ErrorSummary[];
1919
missing: TSLintRuleOptions[];
2020
plugins: Set<string>;
2121
};
@@ -60,14 +60,7 @@ export const convertRules = (
6060

6161
const merger = dependencies.mergers.get(changes.ruleName);
6262
if (merger === undefined) {
63-
failed.push(
64-
new ConversionError(
65-
new Error(
66-
`No merger for multiple output ${changes.ruleName} rule configurations.`,
67-
),
68-
tslintRule,
69-
),
70-
);
63+
failed.push(ConversionError.forMerger(changes.ruleName));
7164
} else {
7265
const existingNotices = existingConversion.notices || [];
7366
const newNotices = newConversion.notices || [];

‎src/rules/converter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { ConversionError } from "../errors/conversionError";
21
import { TSLintRuleOptions } from "./types";
2+
import { ConversionError } from "../errors/conversionError";
33

44
/**
55
* Section of a TSLint rule's options used for conversion.

‎src/rules/converters/member-ordering.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ export const convertMemberOrdering: RuleConverter = () => {
44
return {
55
rules: [
66
{
7-
ruleName: "@typescript-eslint/member-ordering",
7+
ruleName: "some-rule",
88
},
99
],
1010
};

‎src/rules/converters/no-eval.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ export const convertNoEval: RuleConverter = () => {
44
return {
55
rules: [
66
{
7-
ruleName: "no-eval",
7+
ruleName: "some-rule",
88
},
99
],
1010
};

0 commit comments

Comments
 (0)