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

Use --fix instead of -t applyFixes #1697

Merged
merged 4 commits into from
Nov 8, 2016
Merged

Use --fix instead of -t applyFixes #1697

merged 4 commits into from
Nov 8, 2016

Conversation

nchen63
Copy link
Contributor

@nchen63 nchen63 commented Nov 7, 2016

PR checklist

What changes did you make?

When fixes are applied, they are applied first. If a rule triggers a fix, the AST is reloaded to prevent one fix from overwriting another fix. When all fixes are complete, the sourceFile is linted again if there were fixes. That way, we don't show errors that are already fixed and the positions will be correct.

here's what it looks like when using the prose formatter (the only formatter to output that there are fixes) when there is 1 issue fix and 1 remaining issue that can't be fixed:

Fixed 1 error(s) in ../a.ts

../a.ts[6, 1]: Exceeds maximum line length of 140

@nchen63 nchen63 force-pushed the use--fix branch 3 times, most recently from 1efd92d to c9511d1 Compare November 7, 2016 22:36
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Generally looks pretty good. Can you also add a test for the CLI option?

@@ -47,5 +47,5 @@ export interface IFormatterMetadata {
export type ConsumerType = "human" | "machine";

export interface IFormatter {
format(failures: RuleFailure[]): string;
format(failures: RuleFailure[], fixes?: RuleFailure[]): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

is the idea that fixes are only supplied to this method if --fix is enabled in the CLI? can you leave a comment explaining that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@@ -50,6 +50,9 @@ let processed = optimist
alias: "exclude",
describe: "exclude globs from path expansion",
},
fix: {
describe: "Fixes some linting errors (may overwrite linted files)",
Copy link
Contributor

Choose a reason for hiding this comment

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

"some" isn't great language -- can you just use the same description you have on L159

this.failures.push(ruleFailure);
}
// make a 2nd pass if there were any fixes because the positions may be off
if (!this.options.fix || this.fixes.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the first part of this conditional expression looks unintuitive to me -- don't you want to only continue if this.options.fix is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want to run the linter at least once if fix is false. Will try to clarify

@nchen63
Copy link
Contributor Author

nchen63 commented Nov 8, 2016

@adidahiya made changes

@nchen63 nchen63 merged commit d8a5dda into master Nov 8, 2016
@nchen63 nchen63 deleted the use--fix branch November 8, 2016 17:38
@adidahiya
Copy link
Contributor

heads up @alexeagle

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use --fix instead of -t applyFixes
2 participants