Add String Matching Boost for Case Matches #9512
Conversation
|
|
||
| if (str.substr(0, query.length) !== query) { | ||
| // Penalize for not matching case | ||
| result.matchGoodness *= 0.5; |
There was a problem hiding this comment.
Compute a value that is enough to trickle the non-case matches below the case-matches
There was a problem hiding this comment.
nit: result.scoreDebug should probably reflect this.
There was a problem hiding this comment.
I added this below but I'm not 100% certain what should be debugged here. Do we think it's important to just show the beginning value computed due to case match or not or have debug show the MAX_VALUE and a deduction for case. Since it's just a really big number it seems that we don't get much distinction from showing how much was deduced so just having the beginning value was enough for me in my thinking but I wanted to get your opinion.
There was a problem hiding this comment.
I was basically just thinking that scoreDebug should reflect the actual score, so you can get an idea of what you're looking at if you're trying to debug it.
| @@ -732,14 +768,18 @@ define(function (require, exports, module) { | |||
| } | |||
|
|
|||
| // comparisons are case insensitive, so switch to lower case here | |||
| query = query.toLowerCase(); | |||
| var queryStr = query.toLowerCase(); | |||
There was a problem hiding this comment.
I'd recommend changing queryStr to queryLower or something to make it a bit clearer (also changing compareStr to strLower for consistency)
|
The overall approach here seems reasonable. The trick is going to be getting the tests to pass. "should find the right sru" is failing, for example... the scoring tends to be a delicate balance (which is why scoreDebug is helpful!) |
|
@dangoor well technically most of the tests should fail since I modified all of the private function signatures to accept 2 new parameters. Interestingly that only one of them is failing but I haven't spent any time running tests. |
|
@JeffryBooher the ones that test the private bits of StringMatch are failing because of the function signature change. But the ones that test the results at the public API level are the interesting ones. |
JeffryBooher
changed the title
|
@dangoor fixed the tests, added a case test, anything else? |
|
Looks good. I had a couple of minor comments about the code (see above), but otherwise this change looks to fit the bill fine. |
|
@dangoor looks like i had a dangling commit that i didn't push that fixed the code nits. I updated the debug data and pushed and I think it's ready for another review. I can rebase with master if you're ready to merge this. |
|
Looks good to me. This is ready for merging. You could just rebase and merge in one step, if you want. |
|
Closing in lieu of #9615 |
For #3971
Functionvsfunctionhas been bothering me so I thought I'd try to fix it. Spent an hour or so on research and another 45 on implementation which isn't too bad considering I haven't spent much time in that code.Wanted to get some feedback on the approach.
Issues:
CC: @dangoor