Skip to content

Factored config rule conversions into comment conversions #712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion docs/Architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand Down
16 changes: 9 additions & 7 deletions src/comments/convertComments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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({
Expand All @@ -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({
Expand All @@ -71,7 +73,7 @@ describe("convertComments", () => {
});

// Act
const result = await convertComments(dependencies, []);
const result = await convertComments(dependencies, [], new Map());

// Assert
expect(result).toEqual({
Expand All @@ -91,7 +93,7 @@ describe("convertComments", () => {
});

// Act
const result = await convertComments(dependencies, []);
const result = await convertComments(dependencies, ["*.ts"], new Map());

// Assert
expect(result).toEqual({
Expand All @@ -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({
Expand All @@ -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({
Expand Down
5 changes: 3 additions & 2 deletions src/comments/convertComments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
46 changes: 38 additions & 8 deletions src/comments/convertFileComments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand Down
6 changes: 4 additions & 2 deletions src/comments/convertFileComments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -23,7 +24,8 @@ export const convertFileComments = async (
fileContent,
comments,
dependencies.converters,
ruleConversionCache,
ruleCommentsCache,
ruleEquivalents,
);

return fileContent === newFileContent
Expand Down
22 changes: 15 additions & 7 deletions src/comments/replaceFileComments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
1 change: 1 addition & 0 deletions src/conversion/conversionResults.stubs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const createEmptyConversionResults = (
failed: [],
missing: [],
plugins: new Set(),
ruleEquivalents: new Map(),
...overrides,
});

Expand Down
44 changes: 22 additions & 22 deletions src/conversion/convertLintConfig.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ResultStatus, FailedResult } from "../types";
import { createEmptyConversionResults } from "./conversionResults.stubs";
import { convertLintConfig, ConvertLintConfigDependencies } from "./convertLintConfig";

const stubSettings = {
Expand All @@ -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: {
Expand Down
3 changes: 2 additions & 1 deletion src/conversion/convertLintConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down
10 changes: 9 additions & 1 deletion src/rules/convertRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export type RuleConversionResults = {
failed: ErrorSummary[];
missing: TSLintRuleOptions[];
plugins: Set<string>;
ruleEquivalents: Map<string, string[]>;
};

export const convertRules = (
Expand All @@ -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)) {
Expand All @@ -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,
Expand Down Expand Up @@ -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 };
};