search

jellyfin / jellyfin-plugin-anidb

This plugin adds the metadata provider for aniDB to Jellyfin
GNU General Public License v2.0
30 stars 13 forks source link

Verify that Fuzzy matching is working as intended, and add tests #67

Open oddstr13 opened 2 months ago

oddstr13 commented 2 months ago

It is unclear to me whether this code works as expected or not. Review that the generated regex actually is what's intended, and add tests both for the regex generation and the fuzzy matching.

In particular, this looks suspect to me, I would expect the output of this to be ((OVA)|(((OVA)|(OAD)))), due to sequential .replace. https://github.com/jellyfin/jellyfin-plugin-anidb/blob/dd298960b7c211c9f54ecd3502bf677664acf863/Jellyfin.Plugin.AniDB/Providers/equals_check.cs#L86-L87

https://github.com/jellyfin/jellyfin-plugin-anidb/blob/dd298960b7c211c9f54ecd3502bf677664acf863/Jellyfin.Plugin.AniDB/Providers/equals_check.cs#L54-L94

oddstr13 commented 2 months ago

See #56 and #57 for details on what the intended use of this function is.

nalsai commented 2 months ago

OAD stands for Original Animation DVD and is sometimes used interchangeably with OVA.

It is correct that FuzzyRegexEscape turns OVA into ((OVA)|(((OVA)|(OAD)))). c, k, & and and have the same problem.

It isn't really a big problem since it is just redundant but doesn't cause any problems (except for slightly reducing the regex performance).

Something like the unused https://github.com/jellyfin/jellyfin-plugin-anidb/blob/master/Jellyfin.Plugin.AniDB/Providers/AniDB/Identity/AniDbTitleMatcher.cs would ultimately be better than the currently used regex.

Tests should definitely be added.