kellnerd / harmony

Music Metadata Aggregator and MusicBrainz Importer
MIT License
22 stars 6 forks source link

Refactor per region lookup #13

Closed phw closed 3 weeks ago

phw commented 3 weeks ago

This refactors the multi-region lookups in Tidal and iTunes to use a common abstraction.

It implements abstract base classes MetadataApiProvider and ReleaseApiLookup as specializations of the existing abstract classes that hold functionality common to typical API based providers.

MetadataApiProvider for now only enforces the implementation of a query function as we already had it on the Deezer, iTunes and Tidal providers. ReleaseApiLookup provides the helper method queryAllRegions which generalizes performing release queries for each region until a result is found.

queryAllRegions expects a validator callback, which must verify the request response. If the result is usable the validator must return true. If it returns false the next region is tried. There is a optional error validator which can be used to validate exceptions being thrown during query.

Overall this greatly simplifies the request handling in both the iTunes and Deezer provider.

The new base classes will also be suited for adding abstractions for e.g. access token handling a currently present in the Tidal provider.

phw commented 3 weeks ago

We can even remove the CacheEntry.region attribute again which was kind of a hack to let the iTunes provider class return the region to the release lookup class, but I can't create a suggestion for this since you haven't touched that line.

I removed it. The part of the code where the iTunes provider set this attribute on the cache entry looked indeed out of place. But now I understand better why it was there.