bug: FuzzyMatch should only search trough commands related to the incoming update

[carafelix]

Right now fuzzyMatch is searching trough all localized and non-localized versions of the commands.
Example mimicking the inner workings of the fuzzyMatch:

const cmds = new Commands<Context>();
            cmds.command('butcher', 'green beret', () => { })
            cmds.command('duke', 'sniper', () => { }).localize('es', 'duque', 'francotirador')
            cmds.command('fins', 'marine', () => { }).addToScope({ type: "all_private_chats" }, () => { })
            
            const commandsSet = new Set(
                cmds
                    .toArgs()
                    .flatMap((item) => item.commands.map((command) => command.command)),
            );

returns:

Set(4) { "butcher", "duke", "fins", "duque" }

This is important since fuzzyMatch could have matched against a localized/default command version that it's not relevant to the user lang, in other words, returned another foreign local of the command, in any case, one that the end-user is not seeing from the commands menu.
It's basically a collision problem.

A more appropriate search would be to filter only trough the respective ctx.from?.language.scope and opt-out for search-in-all instead.
Must work when no localization exist for a command.
Same thing could be said for the botCommandScope, but that might be irrelevant.

Edit: I have a PoC branch at improveFuzzySearch f782af6 but it's a little bit of a mess. I think I coupled it too much with the changes exposing the prefix (that's needed for correctly recommend the command + it's respective prefix).

Do:

• Test's that demonstrate the issue in a copy of the main branch. see below
• Since from.language.scope it's in IETF language tag, should add a function to assert when it's ISO-639, or convert when possible to ISO-639, if not, fallback to "default". Depends on add languageCode type types#45 approval
• Lower similarity threshold to 0.82

[carafelix]

Another possible solution would be to return all commands that are pass the selected similarity threshold, with their respective language scope. But it could still happen that the best result is not the one for the end-user lang, but at least the dev-user could chose to return the whole list with their scopes, or depending on the user scope return the matching command

[roziscoding]

Maybe we should return the Command classes themselves instead of the name of the command, so the dev could access / use anything they wanted to decide whether to present the command or not 🤔

[carafelix]

I don't think that's the way, since that would require for them to resolve all the logic for knowing which is the localized/default version of the command that is needed to be shown to the end-user, specially if the return of getNearestCommand it's just the command class, it could have matched against any of the command names, you wouldn't know. Needing to access the commands.languages and such, and also the result would be coupled with too much bloat for the use case, in my opinion. Specially because you wouldn't know in advance what the user typo is.

I think in this case it's good to abstract.

I'll write the tests for illustrating the problem. Since as of right now, collisions (in the sense that the shown command it's not from the user lang) could happen not only between localized names that are part from the same command class, but also from ones from different command classes.

I think there are multiple approaches to solve the problem:

• to return a list of all command names that pass the similarity threshold, coupled with the prefix, and expose the lang from which is comming from. Let the dev handle the rest.
• same as above but pre-filter and show only the commands related to the user lang, dev can simply return the highest or all.
• return the one with the highest similarity (like it does right now), but add to it the prefix and expose similarity for comparison with different commands classes (in this case the return of getNearestCommand should also be only 1 string, since the return values of fuzzyMatch are only used internally)

Since this is functionality it's, mostly, a sprinkle to the plugin, a very sweet one tho, I think it's not a bad idea to go with the 3rd option, it's simple and should be okay most of the time. 2nd option it's the most "accurate" for the user experience, but the edge case scenario might be not worth the hassle

[carafelix]

Test's that illustrate the in-class collision problem @roziscoding

commands/test/jaroWrinkler.test.ts

Lines 82 to 127 in ed90dd4

	describe("Should return the command localization related to the user lang", () => {
	const cmds = new Commands<Context>();
	cmds.command("duke", "sniper", () => {})
	.localize("es", "duque", "_")
	.localize("fr", "duc", "_")
	.localize("it", "duca", "_")
	.localize("pt", "duque", "_")
	.localize("de", "herzog", "_")
	.localize("sv", "hertig", "_")
	.localize("da", "hertug", "_")
	.localize("fi", "herttua", "_")
	.localize("hu", "herceg", "_");
	it("sv", () => {
	assertEquals(fuzzyMatch("hertog", cmds, {}), "hertig");
	});
	it("da", () => {
	assertEquals(fuzzyMatch("hertog", cmds, {}), "hertug");
	});
	describe("default", () => {
	it("duke", () =>
	assertEquals(fuzzyMatch("duk", cmds, {}), "duke"));
	it("duke", () =>
	assertEquals(fuzzyMatch("due", cmds, {}), "duke"));
	it("duke", () =>
	assertEquals(fuzzyMatch("dule", cmds, {}), "duke"));
	it("duke", () =>
	assertEquals(fuzzyMatch("duje", cmds, {}), "duke"));
	});
	describe("es", () => {
	it("duque", () =>
	assertEquals(fuzzyMatch("duquw", cmds, {}), "duque"));
	it("duque", () =>
	assertEquals(fuzzyMatch("duqe", cmds, {}), "duque"));
	it("duque", () =>
	assertEquals(fuzzyMatch("duwue", cmds, {}), "duque"));
	});
	describe("fr", () => {
	it("duc", () =>
	assertEquals(fuzzyMatch("duk", cmds, {}), "duc"));
	it("duc", () =>
	assertEquals(fuzzyMatch("duce", cmds, {}), "duc"));
	it("duc", () =>
	assertEquals(fuzzyMatch("ducñ", cmds, {}), "duc"));
	});
	});

Test's that illustrate the between-class collision problem

commands/test/jaroWrinkler.test.ts

Lines 128 to 157 in ed90dd4

	describe("Should return the command localization related to the user lang for similar command names from different command classes", () => {
	const cmds = new Commands<Context>();
	cmds.command("push", "push", () => {})
	.localize("fr", "pousser", "_")
	.localize("pt", "empurrar", "_");
	cmds.command("rest", "rest")
	.localize("fr", "reposer", "_")
	.localize("pt", "poussar", "_");
	describe("pt rest", () => {
	it("poussar", () =>
	assertEquals(fuzzyMatch("pousssr", cmds, {}), "poussar"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("pousar", cmds, {}), "poussar"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("poussqr", cmds, {}), "poussar"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("poussrr", cmds, {}), "poussar"));
	});
	describe("fr push", () => {
	it("poussar", () =>
	assertEquals(fuzzyMatch("pousssr", cmds, {}), "pousser"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("pouser", cmds, {}), "pousser"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("pousrr", cmds, {}), "pousser"));
	it("poussar", () =>
	assertEquals(fuzzyMatch("poussrr", cmds, {}), "pousser"));
	});

[roziscoding]

Since this is functionality it's, mostly, a sprinkle to the plugin, a very sweet one tho, I think it's not a bad idea to go with the 3rd option, it's simple and should be okay most of the time. 2nd option it's the most "accurate" for the user experience, but the edge case scenario might be not worth the hassle

What about going with the third option, but also allowing the dev to decide if they want to consider all commands or only ones from the current scope? By default, it'd match against localized commands only, or default if the user's locale is not found within a command. Then something like ctx.getNearestCommand(commands, { ignoreLocalization: true }) would make the function match against all command locations.

[carafelix]

I merged the test's into the PoC branch. They are passing.

By default, it'd match against localized commands only, or default if the user's locale is not found within a command

That's how I initially impemented it :P

Then something like ctx.getNearestCommand(commands, { ignoreLocalization: true }) would make the function match against all command locations.

I'm on it. Surprisingly, doing the opt-out it's gonna take me some time to do in a more cute fashion, because rn it's a bit too (badly) coupled, but I'll do it right one of this days.

Edit: a working version is at improveFuzzyMatch, it's super ugly code, but I added multiple test's that ensure the desired functionality, please check out those when you have time.
(Would gladly take any advice or guidance for refactoring it)

I'll like to add the feat to compare between commands classes via an overload, should I add it to the same branch?