[Breaking change] Remove createFix, and just use the Replacement(s) as the fix.
#2356
Conversation
| return this.innerRuleName; | ||
| } | ||
|
|
||
| get replacements() { |
There was a problem hiding this comment.
get innerReplacements() { return this.innerInnerReplacements; } 😜
src/language/rule/rule.ts
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
please add an optional sourceFile parameter, that will be passed to node.getStart (for performance)
src/language/rule/rule.ts
Outdated
|
|
||
| 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; |
src/language/walker/ruleWalker.ts
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
I would prefer a rest parameter like createFix had
There was a problem hiding this comment.
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()).
src/language/walker/walkContext.ts
Outdated
| 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); |
There was a problem hiding this comment.
Since you are already changing the style of the code, you could inline this.sourceFile.end here and remove fileLength
There was a problem hiding this comment.
@ajafff is right, there are a lot of stylistic changes here that detract from the PR and make it harder to review. it also increases the chance of merge conflicts as the PR waits for review (as there are now). overall I like this direction and appreciate the PR, but it will help us all to keep it smaller & more focused whenever possible.
|
|
||
| constructor(private sourceFile: ts.SourceFile, | ||
| start: number, | ||
| end: number, | ||
| private failure: string, | ||
| private ruleName: string, | ||
| private fix?: Fix) { | ||
| fix?: Fix) { |
| 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 |
There was a problem hiding this comment.
IMO the inlined fix variable is hard to read, can you save it a variable before calling this method?
|
#2403 merged, so closing. |
PR checklist
Overview of change:
The
Fixclass is not really necessary. It's easier to just pass parameters toaddFailure.This could be viewed as a continuation of #1845. So now instead of
this.addFailure(this.createFailure(..., this.createFix(foo))), you justthis.addFailure(..., foo).CHANGELOG.md entry:
[api] Remove
createFix. Replacements should be passed directly intoaddFailure.