Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add 'dryRun' option to Linter #3371

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ export interface LintResult {
warningCount: number;
failures: RuleFailure[];
fixes?: RuleFailure[];
fixedSources: Record<string, string>;
format: string | FormatterConstructor;
output: string;
}

export interface ILinterOptions {
fix: boolean;
dryRun?: boolean;
formatter?: string | FormatterConstructor;
formattersDirectory?: string;
rulesDirectory?: string | string[];
Expand Down
11 changes: 8 additions & 3 deletions src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class Linter {

private failures: RuleFailure[] = [];
private fixes: RuleFailure[] = [];
private fixedSources: Record<string, string> = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer Map


/**
* Creates a TypeScript program object from a tsconfig.json file path and optional project directory.
Expand Down Expand Up @@ -137,6 +138,7 @@ class Linter {
return {
errorCount,
failures: this.failures,
fixedSources: this.fixedSources,
fixes: this.fixes,
format: formatterName,
output,
Expand All @@ -163,6 +165,7 @@ class Linter {
const fixableFailures = updatedFailures.filter((f) => f.hasFix());
this.fixes = this.fixes.concat(fixableFailures);
source = this.applyFixes(sourceFileName, source, fixableFailures);
this.fixedSources[sourceFileName] = source;
sourceFile = this.getSourceFile(sourceFileName, source);
}
}
Expand All @@ -176,15 +179,17 @@ class Linter {
protected applyFixes(sourceFilePath: string, source: string, fixableFailures: RuleFailure[]): string {
const fixesByFile = createMultiMap(fixableFailures, (f) => [f.getFileName(), f.getFix()!]);
fixesByFile.forEach((fileFixes, filePath) => {
let fileNewSource: string;
let fileNewSource: string | undefined;
if (path.resolve(filePath) === path.resolve(sourceFilePath)) {
source = Replacement.applyFixes(source, fileFixes);
fileNewSource = source;
} else {
} else if (!this.options.dryRun) {
const oldSource = fs.readFileSync(filePath, "utf-8");
fileNewSource = Replacement.applyFixes(oldSource, fileFixes);
}
fs.writeFileSync(filePath, fileNewSource);
if (!this.options.dryRun && typeof fileNewSource === "string") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this basically reverts #2864 when dryRun is true. you don't want that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateProgram is called regardless of dryRun. Not sure what I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateProgram reads the file from disk. With dryRun it is never updated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wow. That's unfortunate. Know any way around that? I think using CompilerHost#getSourceFile could work.

fs.writeFileSync(filePath, fileNewSource);
}
this.updateProgram(filePath);
});

Expand Down
6 changes: 6 additions & 0 deletions src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export interface Options {
*/
config?: string;

/**
* When running with fix: true, do not write files to disk.
*/
dryRun?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only relevant for the CLI. And this doesn't make sense as CLI option


/**
* Exclude globs from path expansion.
*/
Expand Down Expand Up @@ -211,6 +216,7 @@ async function doLinting(
const possibleConfigAbsolutePath = options.config !== undefined ? path.resolve(options.config) : null;
const linter = new Linter(
{
dryRun: !!options.dryRun,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should always be false when running from CLI

fix: !!options.fix,
formatter: options.format,
formattersDirectory: options.formattersDirectory,
Expand Down
12 changes: 12 additions & 0 deletions test/linterTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,16 @@ describe("Linter", () => {
assert.equal(fs.readFileSync(templateFile, "utf-8"), templateDeclarationFixed);
});

it("dry run does not write files to disk", () => {
const linter = new TestLinter({ fix: true, dryRun: true });
const templateFile = createTempFile("ts");
fs.writeFileSync(templateFile, templateDeclaration);
const sourceFile = createSourceFile(templateFile, `${templateDeclaration}`, ScriptTarget.ES2015);
const replacement = new Replacement(6, 9, "");
const failure = new RuleFailure(sourceFile, 6, 15, "Declaration doesn't exist", "foo-bar", replacement);
const result = linter.applyFixesHelper(templateFile, templateDeclaration, [failure]);
assert.equal(fs.readFileSync(templateFile, "utf-8"), templateDeclaration);
assert.equal(result, templateDeclarationFixed);
});

});