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

Add 'dryRun' option to Linter #3371

wants to merge 2 commits into from

Conversation

azz
Copy link

@azz azz commented Oct 22, 2017 •
edited
Loading

PR checklist

Overview of change:

Adds a dryRun option to the Linter constructor. When set, during the fix process, no files will be written to disk. Instead, the result is returned as fixedSources in linter.getResult().

const linter = new Linter({ fix: true, dryRun: true }, program);
linter.lint(filepath, code, config);
const fixed = linter.getResult().fixedSources[filepath];

Is there anything you'd like reviewers to focus on?

Supports multiple calls to lint(), but fixes in other files are ignored. Is that OK?

CHANGELOG.md entry:

[api] add dryRun to Linter.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @azz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@azz azz mentioned this pull request Oct 22, 2017
Closed
ajafff
ajafff suggested changes Oct 22, 2017
View reviewed changes
Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

Thanks for the effort you put into this PR.
Unfortunately I still don't think this is a good idea as mentioned in the feature request.

src/runner.ts Outdated
@@ -41,6 +41,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

src/runner.ts Outdated
@@ -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

src/linter.ts
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.

src/linter.ts Outdated
@@ -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

@ajafff
Copy link
Contributor

ajafff commented Oct 22, 2017

@azz can you please share your use case in #1760?

@azz azz mentioned this pull request Nov 8, 2017
Closed
@havenchyk
Copy link

@ajafff @azz dear contributors, could you please share the status of this PR, what's need to be done here?

@jkillian
Copy link
Contributor

jkillian commented May 1, 2018

#1760 (comment) and #1760 (comment) explain some of the reasons here why dryRun probably isn't the right abstraction. Some possible ways in #1760 how we could possibly improve things though

@jkillian jkillian closed this May 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ajafff ajafff ajafff requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add dry-run to Linter API fix option
5 participants
@azz @palantirtech @ajafff @havenchyk @jkillian