kellnerd / harmony

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

Spotify provider #22

Closed phw closed 2 weeks ago

phw commented 2 weeks ago

This implements a Spotify provider (closes #16).

Similar to Tidal this requires API credentials, which can be setup on https://developer.spotify.com/dashboard/ . The API key and secret must be provided by the environment variables HARMONY_SPOTIFY_CLIENT_ID and HARMONY_SPOTIFY_CLIENT_SECRET.

I addressed all the points I brought up in #16. About the linked track thing see my comment https://github.com/kellnerd/harmony/issues/16#issuecomment-2156075188

Loading a release with more than 50 tracks requires additional requests. This is a large one with 100 tracks across 7 media: https://open.spotify.com/intl-de/album/2ey9jImi467qEu67fvW1kP

The copyright info is returned differently:

The provider normalizes all cases to start with the © or ℗ symbol, depending on the type.

A few things we might consider for future refinement, but which I think should be put into separate PRs:

phw commented 2 weeks ago

We can factor out the copyright normalization logic and reuse it for other providers, e.g. as suggested for Tidal in https://community.metabrainz.org/t/harmony-music-metadata-aggregator-and-musicbrainz-importer/698641/15

But I'd do this after merging this PR. It also needs some further research. I know Tidal includes the copyright text both with and without the © symbol. What I'm unsure is whether this strictly contains copyright © info, or whether it also can sometimes contain phonographic copyright ℗ info. Spotify has those separated, which makes it easier.

phw commented 2 weeks ago

Maybe we even have to add the 13 digit case back, we will see. In any case padding to 13 digits should be the last attempt as it is the rarest case.

I added it back in 75fe532daddd364af35cd3a403839b66909e2f5e, but refactored the code to be more readable and also try 13 character padding last. This should handle the eventual case of 13 characters without complicating the code or requiring unnecessary requests.

kellnerd commented 2 weeks ago

I have also tested the behavior if no credentials are specified and did a similar improvement for the Tidal provider to show the underlying error instead of the generic one: image