Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FuzzyMatcher more convenient to use #1583

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Soreepeong
Copy link
Contributor

Removed the requirements to accommodate ref struct from the invoker code. Can be used as an extension method, with the shortest form of haystack.FuzzyMatchesParts(needle).

@Soreepeong Soreepeong requested a review from a team as a code owner December 21, 2023 14:16
@nebel
Copy link
Contributor

nebel commented Dec 21, 2023

If we're removing methods which return an int (allowing a sort by score) then the internal logic can be greatly simplified. For example FindReverse is not needed at all, since its purpose is to optimise for score but we already know it matches after FindForward returns a valid match position. At which point you can probably remove most of the logic within FindForward as well since scoring then doesn't matter, and CalculateRawScore etc are also unnecessary.

@Soreepeong
Copy link
Contributor Author

I actually considered removing the scoring logic, but it's still a pretty quick operation and I thought some future features may involve sorting by score. I think leaving it in would be fine; they'll just have to use the longer variant:

haystack.FuzzyMatches("needle", Mode.FuzzyParts, CultureInfo.InvariantCulture, out var score)

@nebel
Copy link
Contributor

nebel commented Dec 21, 2023

Oh, my mistake, we do still keep score as an out variable, but it's unused in Dalamud code.

@Soreepeong
Copy link
Contributor Author

Soreepeong commented Dec 21, 2023

Additional thoughts: if we're to implement features to add localized names for plugins, CJK in particular can use extra fuzzy matching which would justify the use of precalculated data for fuzzy matching, such as precalculating which qwerty key sequence is equivalent to producing IME-processed characters. But I don't see that happening anytime soon, so that can be considered way later.

@MidoriKami
Copy link
Contributor

I think score is useful, I personally struggle to use it though because it doesn't really have any definition or description.
I don't really want to have to change a number, rebuild, relaunch, reinject, test, just to see if the number I picked did anything.

I think it might be more useful if it was either defined what the values mean, or if it was converted into a float to represent %match or something along those lines.

@KazWolfe KazWolfe self-assigned this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants