-
Notifications
You must be signed in to change notification settings - Fork 889
Refactor RuleWalker and friends #1900
Comments
These changes seem to be going in the right direction. I would also like to suggest the following: In the class CustomRuleWalker extends SyntaxWalker {
public readonly sourceFile: ts.SourceFile;
public readonly sourceText: string;
public readonly limit: number;
public readonly options: any[];
private failures: RuleFailure[];
private disabledIntervals: IDisabledInterval[];
private ruleName: string;
constructor(sourceFile: ts.SourceFile, options: IOptions) {
super();
this.sourceFile = sourceFile;
this.limit = sourceFile.text.length;
this.sourceText = sourceFile.text.substring(0, this.limit);
this.options = options.ruleArguments;
this.failures = [];
this.disabledIntervals = options.disabledIntervals;
this.ruleName = options.ruleName;
}
public substring(start: number, end: number) {
return this.sourceText.substring(start, end - start);
}
public getLineAndCharacter(node: ts.Node, byEndLocation: boolean = false) {
const index = byEndLocation ? node.getEnd() : node.getStart();
return this.sourceFile.getLineAndCharacterOfPosition(index);
}
public getLine(node: ts.Node, byEndLocation: boolean = false) {
return this.getLineAndCharacter(node, byEndLocation).line;
}
public getLineAndCharacterOfPosition(position: number): ts.LineAndCharacter {
return this.sourceFile.getLineAndCharacterOfPosition(position);
}
public isSingleLineNode(node: ts.Node): boolean {
if (node.kind === ts.SyntaxKind.SyntaxList) {
return node.getFullText(this.sourceFile).indexOf('\n') === -1;
}
return node.getText(this.sourceFile).indexOf('\n') === -1;
} I would not expect the If the |
On a related note, it seems that use of a walker makes AST traversal much slower. (https://gist.github.com/andy-hanson/e9e5c3b33c6514df7825932704603fee) |
Catching up on this issue: abstract class MinimalRuleWalker implements IWalker {
public readonly sourceFile: ts.SourceFile;
public readonly options: any[];
public readonly ruleName: string;
public getSourceFile() {} // for compatibility with the interface and RuleWalker
public getFailures() {}
public addFailureAtNode(...) {}
public addFailureAt(...) {} // plus the other addFailure* methods
public createReplacement(...) {}
public createFix(...) {}
public hasOption(...) {}
public abstract walk(sourceFile: ts.SourceFile);
} Implementing a If you provide positive feedback on this, I'll submit a PR soon(ish). |
@ajafff can remove skip and position now |
This is more like a tracking issue. If you are ok with the todos below, I would like to do the refactoring by myself.
Move funtionality to make it reusable:
Why? I think RuleWalker is too bloated for the common use case. There are all those unnecessary visitor methods on the call stack and everytime
walkChildren
is called, there is a new closure created.Don't get me wrong: this is pretty convenient for beginners and I don't want to rebuild this from scratch. I just want to use my own and do the AST walking myself. But there is some functionality, which I need and which would belong to other modules or classes in my opinion.
isScopeBoundary
andisBlockScopeBoundary
toutils
addFailure
, failure deduplication (existsFailure
) anddisabledIntervals
-check toRule
Of course
RuleWalker
should still work the same to be backwards compatible.Cleanup:
skip()
andposition
property, as they seem to be not useful at allPerformance:
ScopeAwareRuleWalker#getAllScopes()
(BlockScopeAwareRuleWalker#getAllBlockScopes()
doesn't do this either)sourceFile
argument tonode.getStart()
,getWidth()
,getText()
, etc. (see Provide getStart method for RuleWalker #1747)Convenience:
Fix
and useruleName
fromoptions
The text was updated successfully, but these errors were encountered: