Refactor some rules

[ajafff]

PR checklist

• Addresses an existing issue: #0000
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

Refactored some of the rather simple rules to WalkContext or AbstractWalker.
no-trailing-whitespace is the only rule where the logic changes: now it looks for trailing whitespace in the source text and iff there are lines found, it searches the AST for template spans and comments.
[rule-change] no-trailing-whitespace now checks template strings by default. Use the new options ignore-template-strings to restore the old behavior.
[new-rule-option] added ignore-template-strings to no-trailing-whitespace
[enhancement] fixer of quotemark unescapes original quotemark (e.g. '\'' -> "'")

Refactoring is not done yet, but the perf improvement already looks pretty good: (linting this project with itself):
master: 7.7s
this PR: 6.7s

[jmlopez-rod]

@ajafff how do you get the timings? Are you taking averages of runs or just a sample? I'm trying to see what's the best way of measuring performance on my rules so that I can change them to using the abstract walker but I can't really get a hold of a number.

[andy-hanson]

Make options an optional parameter instead of using undefined

[andy-hanson]

Never mind, it doesn't seem like there's any way to do this in a type-safe way.

[andy-hanson]

Can do it with just a regexp:

const rgx = /[^\S\n]+(\r?\n|$)/g; // Non-newline spaces, then a newline or EOF
let match: RegExpExecArray | null;
while (match = rgx.exec(text)) {
    const end = text.indexOf("\n", match.index);
    possibleFailures.push({
        pos: match.index,
        end: end === -1 ? text.length : end
    });
}

[ajafff]

Your solution looks nicer at first glance. It seems to include \r in the range of the failure, though.

I just added a new property contentLength to getLineRanges which has already stripped \r?\n. The regex is now even simpler.

[andy-hanson]

It can be done without indexOf because enough information is returned by exec. Then it's easier to use len instead of end, so you end up with:

function walk(ctx: Lint.WalkContext<IgnoreOption>) {
    const { options, sourceFile } = ctx;
    const possibleFailures = getPossibleFailures(sourceFile.text);
    if (possibleFailures.length === 0) {
        return;
    }

    const excludedRanges = options === IgnoreOption.None
                           ? getTemplateRanges(sourceFile)
                           : getExcludedRanges(sourceFile, options);
    for (const { pos, len } of possibleFailures) {
        if (!excludedRanges.some((range) => range.pos < pos && pos < range.end)) {
            ctx.addFailureAt(pos, len, Rule.FAILURE_STRING, ctx.createFix(Lint.Replacement.deleteText(pos, len)));
        }
    }
}

function getPossibleFailures(text: string): Array<{ pos: number, len: number }> {
    const result = [];
    const rgx = /[^\S\n]+(\r?\n|$)/g;
    let match: RegExpExecArray | null;
    while (match = rgx.exec(text)) {
        const matched = match[0];
        result.push({ pos: match.index, len: matched.endsWith("\n") ? matched.length - 1 : matched.length });
    }
    return result;
}

[andy-hanson]

Also, could this rule have an option to not ignore template strings?

[ajafff]

@jmlopez-rod The easy way is to execute yarn lint:from-bin (or something similar like time bin/tslint ...) multiple times the get the average. Most of the time the measured times don't differ much. Then build the revision you want to compare to and execute it again.

When you want to measure the time without I/O, you need to use the profiler in node inspector: node --debug-brk --inspect path/to/tslint

[andy-hanson]

This is why I don't like object-literal-sort-keys: It doesn't make sense to have the keys in alphabetical order here.

[ajafff]

@andy-hanson I changed no-trailing-whitespace to check template strings by default and added an option to ignore them.

[adidahiya]

👍 for trailing whitespace behavior change

[adidahiya]

multiple tsutils again :(

I've noticed yarn do this often lately when I try to upgrade a package (seems like a bug). I've usually fixed it by deleting all of the entries for a particular package (tsutils here) and running yarn again; it then resolves them to a single version.

[adidahiya]

oh, and yarn.lock has a conflict anyway.

otherwise the PR looks good to go

[ajafff]

@adidahiya conflicts in yarn.lock are resolved now