FIX @W-19772057@ Refactored selection algorithm

[jfeingold35]

Previously, eslint:(2,BestPractices:(3,4)) was being parsed as eslint:((2,BestPractices):(3,4)) instead of eslint:(2,(BestPractices:(3,4))). This has been corrected, and additional test coverage has been added.

[stephen-carter-at-sf]

Instead of keeping track of the commaIdx, since commas trump colons, can't we just do a quick return here to avoid processing more. I only think we need to keep track of the colonIdx the whole way through.

That is, can't we just do:

} else if (parenBalance > 0) { // Ignore chars inside of parens
    continue;
} else if (char === ',') {
        const left: string = trimmedSelector.slice(0, i);
        const right: string = trimmedSelector.slice(i + 1);
        return new OrSelector(toSelector(left), toSelector(right));
} else if (char === ':' && colonIdx === null) {
    colonIdx = i;
}

Then you don't need lines 47-50 below.

Also maybe rename colonIdx to firstColonIdx

[jfeingold35]

I thought about that, but doing the full iteration allows us to catch situations where the number of open-parens exceeds the number of close-parens, since the balance will be positive at the end.

[stephen-carter-at-sf]

You will catch it anyway already because all the parens need to be balanced on both sides of the top level comma anyway and so it'll get caught in the next iteration of the loop.... so I do believe that you can indeed short circuit when you find the first top level comma.

[stephen-carter-at-sf]

When using includes or regex testing, you are increasing the runtime since you are doing additional passes unnecessarily. We want this part of the code to be as performant as possible. So you should move the space check as in if statement inside of the loop since you are already checking character by character.

[stephen-carter-at-sf]

Likewise, you can test the first and last character of each selectorString (since the algorithm is recursive) by just checking the first and last characters without a regular expression.

So

} else if (selectorString.startsWith(',') ||  selectorString.startsWith(':') || selectorString.endsWith(',') ||  selectorString.endsWith(':')) {

would be better here.

Or better yet... you could just add it to the if statement in your loop below. For example

    if (i===0 && (char === ',' || char === ':'))  {

.

If this wasn't a recursive algorithm where the operations get multiplied then it wouldn't be as big of a deal - but it is, so we need to try to keep O(1) operations inside of a single O(n) loop if possible.

[stephen-carter-at-sf]

I'd still consider reducing complexity by removing the nested if statements. Consider:

} else if (parenBalance > 0) { // Ignore chars inside of parens
    continue;
} else if (char === ',') {
    const left: string = trimmedSelector.slice(0, i);
    const right: string = trimmedSelector.slice(i + 1);
    return new OrSelector(toSelector(left), toSelector(right));
} else if (char === ':' && firstColonIdx === null) {
    firstColonIdx = i;
}

But approved.