search

grammyjs / commands

Work-in-progress plugin for managing commands with grammY.
https://grammy.dev
MIT License
13 stars 2 forks source link

Switch to @std for closest string #48

Closed KnorpelSenf closed 4 weeks ago

KnorpelSenf commented 1 month ago

I think none of us really was aware of https://jsr.io/@std/text/doc/~/closestString and I think we might want to swap out our own implementation for theirs.

Perhaps we should bench it and then contribute back our implementation if it turns out that we're faster. That being said, their code has more bit operators so based on this highly professional metric(TM) I claim with confidence that they did a better job.

roziscoding commented 4 weeks ago

It does use a different algorithm and, as per what I could find, Jaro Winkler (which we use today) would be better suited for smaller words (like command names). I don't have a strong opinion nor have I done extensive research, so if you and @carafelix think the change is appropriate, I'm okay with implementing it.

KnorpelSenf commented 4 weeks ago

Makes sense. We should bench it for command names before doing anything about this.

carafelix commented 4 weeks ago

By default, calculates the distance between words using the Levenshtein distance.

That implies that the algorithm being use can be changed, haven't checked, but if it does not allow for Jaro-Winkler, I'll say we stick to our implementation. The point being that JW prioritizes similarities found at the beginning of the word. Example: If the word to be found is "HOLA". "HILA" and "HOLAE" would both have 1 Levenshtein distance from the word "HOLA". JW instead would prefer "HOLAE" because it matches more characters at the beginning of the word.

That's relevant because humans tend to do more typing mistakes at the end of the word that in the beginning.

In any case, this is a common problem, there should be enough literature about it to make an informed decision.

KnorpelSenf commented 4 weeks ago

Oh that's very interesting. I was under the assumption that both implementations yielded identical results. I no longer think it's good to share code here. Thanks for enlightening me!

carafelix commented 4 weeks ago

That implies that the algorithm being use can be changed

It is possible to pass a compareFn to closestString. Still worth considering (?)

KnorpelSenf commented 4 weeks ago

No. Adding a dep for 5 trivial lines of code isn't worth it.