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

[Breaking change] Remove createFix, and just use the Replacement(s) as the fix. #2356

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
21 changes: 9 additions & 12 deletions src/language/rule/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ export class Replacement {
return replacements.reduce((text, r) => r.apply(text), content);
}

public static replaceNode(node: ts.Node, text: string): Replacement {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add an optional sourceFile parameter, that will be passed to node.getStart (for performance)

return this.replaceFromTo(node.getStart(), node.getEnd(), text);
}

public static replaceFromTo(start: number, end: number, text: string) {
return new Replacement(start, end - start, text);
}
Expand Down Expand Up @@ -188,19 +192,10 @@ export class Fix {
return Replacement.applyAll(content, replacements);
}

constructor(private innerRuleName: string, private innerReplacements: Replacement[]) {
}

get ruleName() {
return this.innerRuleName;
}

get replacements() {
return this.innerReplacements;
}
constructor(readonly replacements: Replacement[]) {}

public apply(content: string) {
return Replacement.applyAll(content, this.innerReplacements);
return Replacement.applyAll(content, this.replacements);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

get innerReplacements() { return this.innerInnerReplacements; } 😜

}
}

Expand Down Expand Up @@ -240,19 +235,21 @@ export class RuleFailure {
private endPosition: RuleFailurePosition;
private rawLines: string;
private ruleSeverity: RuleSeverity;
private fix?: Fix;

constructor(private sourceFile: ts.SourceFile,
start: number,
end: number,
private failure: string,
private ruleName: string,
private fix?: Fix) {
fix?: Replacement | Replacement[]) {

this.fileName = sourceFile.fileName;
this.startPosition = this.createFailurePosition(start);
this.endPosition = this.createFailurePosition(end);
this.rawLines = sourceFile.text;
this.ruleSeverity = "error";
this.fix = fix ? new Fix(Array.isArray(fix) ? fix : [fix]) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

use arrayify?

}

public getFileName() {
Expand Down
14 changes: 5 additions & 9 deletions src/language/walker/ruleWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import * as ts from "typescript";

import {Fix, IOptions, Replacement, RuleFailure} from "../rule/rule";
import {IOptions, Replacement, RuleFailure} from "../rule/rule";
import {SyntaxWalker} from "./syntaxWalker";
import {IWalker} from "./walker";

Expand Down Expand Up @@ -65,7 +65,7 @@ export class RuleWalker extends SyntaxWalker implements IWalker {
}

/** @deprecated Prefer `addFailureAt` and its variants. */
public createFailure(start: number, width: number, failure: string, fix?: Fix): RuleFailure {
public createFailure(start: number, width: number, failure: string, fix?: Replacement | Replacement[]): RuleFailure {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a rest parameter like createFix had

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that at first, but that can make life difficult if you have a function returning optional fixes. You would need to use const fix = optionalFix(); ctx.addFailureAtNode(node, failure, ...(fix ? fix : [])), instead of just ctx.addFailureAtNode(node, failure, optionalFix()).

const from = (start > this.limit) ? this.limit : start;
const to = ((start + width) > this.limit) ? this.limit : (start + width);
return new RuleFailure(this.sourceFile, from, to, failure, this.ruleName, fix);
Expand All @@ -77,17 +77,17 @@ export class RuleWalker extends SyntaxWalker implements IWalker {
}

/** Add a failure with any arbitrary span. Prefer `addFailureAtNode` if possible. */
public addFailureAt(start: number, width: number, failure: string, fix?: Fix) {
public addFailureAt(start: number, width: number, failure: string, fix?: Replacement | Replacement[]) {
this.addFailure(this.createFailure(start, width, failure, fix));
}

/** Like `addFailureAt` but uses start and end instead of start and width. */
public addFailureFromStartToEnd(start: number, end: number, failure: string, fix?: Fix) {
public addFailureFromStartToEnd(start: number, end: number, failure: string, fix?: Replacement | Replacement[]) {
this.addFailureAt(start, end - start, failure, fix);
}

/** Add a failure using a node's span. */
public addFailureAtNode(node: ts.Node, failure: string, fix?: Fix) {
public addFailureAtNode(node: ts.Node, failure: string, fix?: Replacement | Replacement[]) {
this.addFailureAt(node.getStart(this.sourceFile), node.getWidth(this.sourceFile), failure, fix);
}

Expand All @@ -110,8 +110,4 @@ export class RuleWalker extends SyntaxWalker implements IWalker {
public getRuleName(): string {
return this.ruleName;
}

public createFix(...replacements: Replacement[]): Fix {
return new Fix(this.ruleName, replacements);
}
}
18 changes: 7 additions & 11 deletions src/language/walker/walkContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,27 @@

import * as ts from "typescript";

import { Fix, Replacement, RuleFailure } from "../rule/rule";
import { Replacement, RuleFailure } from "../rule/rule";

export class WalkContext<T> {
public readonly failures: RuleFailure[] = [];

constructor(public readonly sourceFile: ts.SourceFile, public readonly ruleName: string, public readonly options: T) {}

/** Add a failure with any arbitrary span. Prefer `addFailureAtNode` if possible. */
public addFailureAt(start: number, width: number, failure: string, fix?: Fix) {
public addFailureAt(start: number, width: number, failure: string, fix?: Replacement | Replacement[]) {
this.addFailure(start, start + width, failure, fix);
}

public addFailure(start: number, end: number, failure: string, fix?: Fix) {
public addFailure(start: number, end: number, failure: string, fix?: Replacement | Replacement[]) {
const fileLength = this.sourceFile.end;
this.failures.push(
new RuleFailure(this.sourceFile, Math.min(start, fileLength), Math.min(end, fileLength), failure, this.ruleName, fix),
);
start = Math.min(start, fileLength);
end = Math.min(end, fileLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are already changing the style of the code, you could inline this.sourceFile.end here and remove fileLength

this.failures.push(new RuleFailure(this.sourceFile, start, end, failure, this.ruleName, fix));
}

/** Add a failure using a node's span. */
public addFailureAtNode(node: ts.Node, failure: string, fix?: Fix) {
public addFailureAtNode(node: ts.Node, failure: string, fix?: Replacement | Replacement[]) {
this.addFailure(node.getStart(this.sourceFile), node.getEnd(), failure, fix);
}

public createFix(...replacements: Replacement[]) {
return new Fix(this.ruleName, replacements);
}
}
10 changes: 3 additions & 7 deletions src/rules/alignRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,9 @@ class AlignWalker extends Lint.RuleWalker {
const curPos = this.getLineAndCharacterOfPosition(node.getStart());
if (curPos.line !== prevPos.line && curPos.character !== alignToColumn) {
const diff = alignToColumn - curPos.character;
let fix;
if (0 < diff) {
fix = this.createFix(this.appendText(node.getStart(), " ".repeat(diff)));
} else {
fix = this.createFix(this.deleteText(node.getStart() + diff, -diff));
}
this.addFailureAtNode(node, kind + Rule.FAILURE_STRING_SUFFIX, fix);
this.addFailureAtNode(node, kind + Rule.FAILURE_STRING_SUFFIX, 0 < diff
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the inlined fix variable is hard to read, can you save it a variable before calling this method?

? this.appendText(node.getStart(), " ".repeat(diff))
: this.deleteText(node.getStart() + diff, -diff));
}
prevPos = curPos;
}
Expand Down
13 changes: 5 additions & 8 deletions src/rules/arrayTypeRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,11 @@ class ArrayTypeWalker extends Lint.RuleWalker {
// Add a space if the type is preceded by 'as' and the node has no leading whitespace
const space = !parens && node.parent!.kind === ts.SyntaxKind.AsExpression &&
node.getStart() === node.getFullStart() ? " " : "";
const fix = this.createFix(
this.addFailureAtNode(node, failureString, [
this.createReplacement(typeName.getStart(), parens, space + "Array<"),
// Delete the square brackets and replace with an angle bracket
this.createReplacement(typeName.getEnd() - parens, node.getEnd() - typeName.getEnd() + parens, ">"),
);
this.addFailureAtNode(node, failureString, fix);
]);
}

super.visitArrayType(node);
Expand All @@ -83,21 +82,19 @@ class ArrayTypeWalker extends Lint.RuleWalker {
const typeArgs = node.typeArguments;
if (!typeArgs || typeArgs.length === 0) {
// Create an 'any' array
const fix = this.createFix(this.createReplacement(node.getStart(), node.getWidth(), "any[]"));
this.addFailureAtNode(node, failureString, fix);
this.addFailureAtNode(node, failureString, this.createReplacement(node.getStart(), node.getWidth(), "any[]"));
} else if (typeArgs && typeArgs.length === 1 && (!this.hasOption(OPTION_ARRAY_SIMPLE) || this.isSimpleType(typeArgs[0]))) {
const type = typeArgs[0];
const typeStart = type.getStart();
const typeEnd = type.getEnd();
const parens = type.kind === ts.SyntaxKind.UnionType ||
type.kind === ts.SyntaxKind.FunctionType || type.kind === ts.SyntaxKind.IntersectionType;
const fix = this.createFix(
this.addFailureAtNode(node, failureString, [
// Delete Array and the first angle bracket
this.createReplacement(node.getStart(), typeStart - node.getStart(), parens ? "(" : ""),
// Delete the last angle bracket and replace with square brackets
this.createReplacement(typeEnd, node.getEnd() - typeEnd, (parens ? ")" : "") + "[]"),
);
this.addFailureAtNode(node, failureString, fix);
]);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/rules/arrowParensRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ function walk(ctx: Lint.WalkContext<Options>) {
const parameter = node.parameters[0];
const start = parameter.getStart(ctx.sourceFile);
const end = parameter.end;
ctx.addFailure(start, end, Rule.FAILURE_STRING_MISSING, ctx.createFix(
ctx.addFailure(start, end, Rule.FAILURE_STRING_MISSING, [
Lint.Replacement.appendText(start, "("),
Lint.Replacement.appendText(end, ")"),
));
]);
}
} else if (ctx.options.banSingleArgParens) {
const closeParen = getChildOfKind(node, ts.SyntaxKind.CloseParenToken)!;
ctx.addFailureAtNode(node.parameters[0], Rule.FAILURE_STRING_EXISTS, ctx.createFix(
ctx.addFailureAtNode(node.parameters[0], Rule.FAILURE_STRING_EXISTS, [
Lint.Replacement.deleteText(openParen.end - 1, 1),
Lint.Replacement.deleteText(closeParen.end - 1, 1),
));
]);
}
}
return ts.forEachChild(node, cb);
Expand Down
7 changes: 4 additions & 3 deletions src/rules/arrowReturnShorthandRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ class Walker extends Lint.RuleWalker {
return getLine(node.getEnd()) > getLine(node.getStart());
}

private createArrowFunctionFix(arrowFunction: ts.FunctionLikeDeclaration, body: ts.Block, expr: ts.Expression): Lint.Fix | undefined {
private createArrowFunctionFix(arrowFunction: ts.FunctionLikeDeclaration, body: ts.Block, expr: ts.Expression,
): Lint.Replacement[] | undefined {
const text = this.getSourceFile().text;
const statement = expr.parent!;
const returnKeyword = Lint.childOfKind(statement, ts.SyntaxKind.ReturnKeyword)!;
Expand All @@ -78,7 +79,7 @@ class Walker extends Lint.RuleWalker {

const anyComments = hasComments(arrow) || hasComments(openBrace) || hasComments(statement) || hasComments(returnKeyword) ||
hasComments(expr) || (semicolon && hasComments(semicolon)) || hasComments(closeBrace);
return anyComments ? undefined : this.createFix(
return anyComments ? undefined : [
// Object literal must be wrapped in `()`
...(expr.kind === ts.SyntaxKind.ObjectLiteralExpression ? [
this.appendText(expr.getStart(), "("),
Expand All @@ -90,7 +91,7 @@ class Walker extends Lint.RuleWalker {
this.deleteFromTo(statement.getStart(), expr.getStart()),
// " }" (may include semicolon)
this.deleteFromTo(expr.end, closeBrace.end),
);
];

function hasComments(node: ts.Node): boolean {
return ts.getTrailingCommentRanges(text, node.getEnd()) !== undefined;
Expand Down
5 changes: 2 additions & 3 deletions src/rules/noAngleBracketTypeAssertionRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,12 @@ export class Rule extends Lint.Rules.AbstractRule {
class NoAngleBracketTypeAssertionWalker extends Lint.RuleWalker {
public visitTypeAssertionExpression(node: ts.TypeAssertion) {
const { expression, type } = node;
const fix = this.createFix(
this.addFailureAtNode(node, Rule.FAILURE_STRING, [
// add 'as' syntax at end
this.createReplacement(node.getEnd(), 0, ` as ${type.getText()}`),
// delete the angle bracket assertion
this.createReplacement(node.getStart(), expression.getStart() - node.getStart(), ""),
);
this.addFailureAtNode(node, Rule.FAILURE_STRING, fix);
]);
super.visitTypeAssertionExpression(node);
}
}
18 changes: 9 additions & 9 deletions src/rules/noAnyRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ export class Rule extends Lint.Rules.AbstractRule {
"or suppress this occurrence.";

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithWalker(new NoAnyWalker(sourceFile, this.getOptions()));
return this.applyWithFunction(sourceFile, walk);
}
}

class NoAnyWalker extends Lint.RuleWalker {
public visitAnyKeyword(node: ts.Node) {
const fix = this.createFix(
this.createReplacement(node.getStart(), node.getWidth(), "{}"),
);
this.addFailureAtNode(node, Rule.FAILURE_STRING, fix);
super.visitAnyKeyword(node);
}
function walk(ctx: Lint.WalkContext<void>): void {
ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void {
if (node.kind === ts.SyntaxKind.AnyKeyword) {
ctx.addFailureAtNode(node, Rule.FAILURE_STRING, Lint.Replacement.replaceNode(node, "{}"));
} else {
return ts.forEachChild(node, cb);
}
});
}
2 changes: 1 addition & 1 deletion src/rules/noBooleanLiteralCompareRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class Walker extends Lint.ProgramAwareRuleWalker {
}
}

this.addFailureAtNode(expression, Rule.FAILURE_STRING(negate), this.createFix(...replacements));
this.addFailureAtNode(expression, Rule.FAILURE_STRING(negate), replacements);
}
}

Expand Down
14 changes: 6 additions & 8 deletions src/rules/noConsecutiveBlankLinesRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,12 @@ function walk(ctx: Lint.WalkContext<number>) {
const templateRanges = getTemplateRanges(ctx.sourceFile);
for (const possibleFailure of possibleFailures) {
if (!templateRanges.some((template) => template.pos < possibleFailure.pos && possibleFailure.pos < template.end)) {
ctx.addFailureAt(possibleFailure.pos, 1, failureString, ctx.createFix(
Lint.Replacement.deleteFromTo(
// special handling for fixing blank lines at the end of the file
// to fix this we need to cut off the line break of the last allowed blank line, too
possibleFailure.end === sourceText.length ? getStartOfLineBreak(sourceText, possibleFailure.pos) : possibleFailure.pos,
possibleFailure.end,
),
));
// special handling for fixing blank lines at the end of the file
// to fix this we need to cut off the line break of the last allowed blank line, too
const start = possibleFailure.end === sourceText.length
? getStartOfLineBreak(sourceText, possibleFailure.pos)
: possibleFailure.pos;
ctx.addFailureAt(possibleFailure.pos, 1, failureString, Lint.Replacement.deleteFromTo(start, possibleFailure.end));
}
}
}
Expand Down
21 changes: 9 additions & 12 deletions src/rules/noInferrableTypesRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,23 +98,23 @@ class NoInferrableTypesWalker extends Lint.AbstractWalker<IOptions> {
return ts.forEachChild(sourceFile, cb);
}

private checkDeclaration(node: ts.VariableLikeDeclaration) {
if (node.type != null && node.initializer != null) {
let failure: string | null = null;
private checkDeclaration({ name, type, initializer }: ts.VariableLikeDeclaration) {
if (type != null && initializer != null) {
let failure: string | undefined;

switch (node.type.kind) {
switch (type.kind) {
case ts.SyntaxKind.BooleanKeyword:
if (node.initializer.kind === ts.SyntaxKind.TrueKeyword || node.initializer.kind === ts.SyntaxKind.FalseKeyword) {
if (initializer.kind === ts.SyntaxKind.TrueKeyword || initializer.kind === ts.SyntaxKind.FalseKeyword) {
failure = "boolean";
}
break;
case ts.SyntaxKind.NumberKeyword:
if (node.initializer.kind === ts.SyntaxKind.NumericLiteral) {
if (initializer.kind === ts.SyntaxKind.NumericLiteral) {
failure = "number";
}
break;
case ts.SyntaxKind.StringKeyword:
switch (node.initializer.kind) {
switch (initializer.kind) {
case ts.SyntaxKind.StringLiteral:
case ts.SyntaxKind.NoSubstitutionTemplateLiteral:
case ts.SyntaxKind.TemplateExpression:
Expand All @@ -128,11 +128,8 @@ class NoInferrableTypesWalker extends Lint.AbstractWalker<IOptions> {
break;
}

if (failure != null) {
this.addFailureAtNode(node.type,
Rule.FAILURE_STRING_FACTORY(failure),
this.createFix(Lint.Replacement.deleteFromTo(node.name.end, node.type.end)),
);
if (failure !== undefined) {
this.addFailureAtNode(type, Rule.FAILURE_STRING_FACTORY(failure), Lint.Replacement.deleteFromTo(name.end, type.end));
}
}
}
Expand Down
7 changes: 2 additions & 5 deletions src/rules/noStringThrowRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,8 @@ class Walker extends Lint.RuleWalker {
public visitThrowStatement(node: ts.ThrowStatement) {
const {expression} = node;
if (this.stringConcatRecursive(expression)) {
const fix = this.createFix(this.createReplacement(expression.getStart(),
expression.getEnd() - expression.getStart(),
`new Error(${expression.getText()})`));
this.addFailure(this.createFailure(
node.getStart(), node.getWidth(), Rule.FAILURE_STRING, fix));
this.addFailureAtNode(node, Rule.FAILURE_STRING,
Lint.Replacement.replaceNode(expression, `new Error(${expression.getText()})`));
}

super.visitThrowStatement(node);
Expand Down
2 changes: 1 addition & 1 deletion src/rules/noTrailingWhitespaceRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,5 +120,5 @@ function walk(ctx: Lint.WalkContext<IgnoreOption>) {
}

function reportFailure(ctx: Lint.WalkContext<IgnoreOption>, start: number, end: number) {
ctx.addFailure(start, end, Rule.FAILURE_STRING, ctx.createFix(Lint.Replacement.deleteFromTo(start, end)));
ctx.addFailure(start, end, Rule.FAILURE_STRING, Lint.Replacement.deleteFromTo(start, end));
}
Loading