Provide more predictable fuzzy matching#11309
Conversation
kmcgrady
added
security-assessment-completed
change:bugfix
✅ PR preview is ready!
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0000%
✅ Coverage change is within normal range. |
raethlein
left a comment
raethlein
left a comment
There was a problem hiding this comment.
LGTM! Just left a minor comment about that we might want to mention that some code portions were originally licensed as MIT
There was a problem hiding this comment.
nit: Could we have a small comment here about why we use -? Something like // we use the negative score here because we want the list to be reversed. We don't reverse it in the end because that can lead to UI shuffling. (if I understood it correctly from your PR description)
There was a problem hiding this comment.
| /* App/Models/Order is better than App/MOdels/zRder */ | |
| /* app/models/order is better than app/models/zrder */ |
nit: Could you leave a comment here about the why it is better? I assume its because the o in order comes before the z in zrder.
There was a problem hiding this comment.
nit: if these tests are copied 1:1 from fuzzy, I think we should mention it here
frontend/lib/src/util/fuzzySearch.ts
Outdated
There was a problem hiding this comment.
if this is a substantial copy of the code, I think we should keep the MIT license notice or mention the MIT-licensing here.
kmcgrady
force-pushed
the
fix/more-predictable-fuzzy-matching
branch
from
e876a2c to
affba46
Compare
## Describe your changes When working on #11309, I was working to provide more predictable case sensitive search. It works in some cases (where the case sensitive should come before a case insensitive), but it made fuzzy searching across other instances worse (See #11724) In the end, we will go for case insensitive search, because that's a little more intuitive as users type and will be less great from a case sensitivity. I think the scoring could be better in that case insensitive can be slightly behind a case sensitive match, but that's a much tougher problem to solve. This will alleviate user's concerns ## GitHub Issue Link (if applicable) closes #11724 ## Testing Plan - Added a unit test based on the issue's use case --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
2 participants
Describe your changes
This change migrates away from fzy.js to vendored code with small modifications/improvements including types, and handling case sensitivity (It's MIT licensed).
Also minor, but the sorting for the options is using the negative filter rather than reversing the list. This keeps the sorting more stable and not move options completely around (minor perf benefit).
GitHub Issue Link (if applicable)
closes #11218
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.