Increase the boost for prefix matches. #3493
Conversation
This is a fix for #3411 and is going to be useful for code hinting
ghost
assigned 
| // of the name | ||
| if (c - numConsecutive === lastSegmentStart) { | ||
| newPoints += BEGINNING_OF_NAME_POINTS; | ||
| } |
There was a problem hiding this comment.
I'm a little confused about how to interpret numConsecutive. If I'm reading the code correctly, it only means "number of consecutive characters" if the consecutive run started on a special char -- otherwise it is always 1.
I think the code here still works correctly, since segment start is always a special (I can't think of any way to get a false positive or negative). But it does make the code more confusing.
What if we change it so numConsecutive is always incremented, so it means what it says, and then have something like:
var pointMultiplier = currentRangeStartedOnSpecial ? numConsecutive : 1;
...
newPoints += CONSECUTIVE_MATCHES_POINTS * pointMultiplier;
|
I found a couple cases where the order seems worse than before:
I think you could fix the first two by limiting the new bonus to just the first match range. That should preserve the contiguous-prefix weighting we want for code hinting. The last case doesn't bother me as much since the ideal match only fell one slot and the one above it isn't that different. I think for cases like that we could eventually give a bonus to consecutive special matches that leave no unmatched specials between them (e.g. credit them with some percentage of the bonus you'd get for a contiguous match spanning all the chars between those two specials, since the way we're interpreting it that's what the user meant to match when they typed the two specials in a row). But certainly not needed now :-) |
|
@dangoor done reviewing |
In the process, I tweaked the bonuses to keep things ordered well.
|
Thanks for the feedback! I think that the meaning of My tweaks fixed the "trange" case that you cite, and I added a test case for that. I agree that the "fim" case is not clearly in favor of one over the other and we could probably fashion a boost that helps there.. "smatch" is trickier. My tweaks moved StringMatch to 3rd place, giving the edge to matches that have the "s" at the beginning and "match" at the beginning of the name. Though we know that almost everything we care about is in the "src" directory and don't care much about that "s" match at the beginning, that wouldn't necessarily be true of other projects and those top two matches for "smatch" are perfectly reasonable. I like the idea of a "contiguous specials" bonus, but I worry that such a bonus would push "scrollTop" back over "str" when searching for "st". It makes me wonder if there should be some small amount of configuration of stringMatch scoring so that the rules could be tweaked for code hinting vs. filename matching. Or, maybe the rules are different if the string being matched is just a name without a path? |
| if (DEBUG_SCORES) { | ||
| scoreDebug.beginning += BEGINNING_OF_NAME_POINTS; | ||
| } | ||
| newPoints += BEGINNING_OF_NAME_POINTS; |
There was a problem hiding this comment.
Nit: the order of these two additions is the opposite of the other pairs here.
There was a problem hiding this comment.
Oops sorry, that comment was intended for the next block of code, on line 579...
|
@dangoor New scoring works great for me! Two nits above but that's it. |
Per review feedback
|
@peterflynn Good suggestions! It does make the code more readable. Changes implemented in 1552890 |
|
Sweet! Merging. |
Increase the boost for contiguous matches that make up a prefix
This is a fix for #3411 and is going to be useful for code hinting.
I'll note that this also improves an older test case where "jsu" matches JSLintUtils.js before "JSUtils.js".