Extract code for creating similar artists to a separate class

[sferra]

Addresses #1558.

This PR isolates the code for creating SimilarArtists from the Discogs metadata provider with the intent to reuse it in other metadata plugins which do not properly resolve similar artist images properly.

The first commit represents a draft of how the extracted code could be used (as a separate instance field).
The field could also be added in the MetaProvider class, since all plugins would use it.

IMO we could also (in a separate step) isolate all LastFm-related metadata operations to a separate class, to make them safe and consistent and tested.

@nukeop , I need your feedback. Should we proceed by reusing the code as a field? Each metadata provider would implement the same field, which looks kind of redundant. Or do you have a different pattern in mind?

Also, I locally adjusted the affected metadata providers and they work fine. Since the adjustments are simple, I could add them to this PR, or create a separate PRs if it's easier for you to reveiw - I have no personal preference.

[codecov]

Codecov Report

Attention: Patch coverage is 80.48780% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 72.08%. Comparing base (f1e639e) to head (f42bda2).

❗ Current head f42bda2 differs from pull request most recent head b778009. Consider uploading reports for the commit b778009 to get more accurate results

Files	Patch %	Lines
packages/core/src/plugins/meta/audius.ts	33.33%	2 Missing ⚠️
packages/core/src/plugins/meta/bandcamp.ts	33.33%	2 Missing ⚠️
packages/core/src/plugins/meta/musicbrainz.ts	33.33%	2 Missing ⚠️
packages/core/src/plugins/meta/discogs.ts	66.66%	1 Missing ⚠️
packages/core/src/plugins/meta/itunesmusic.ts	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   72.09%   72.08%   -0.01%     
==========================================
  Files         362      363       +1     
  Lines        6754     6761       +7     
  Branches      499      496       -3     
==========================================
+ Hits         4869     4874       +5     
- Misses       1476     1483       +7     
+ Partials      409      404       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sferra]

I'm on standby on this topic and waiting for feedback.
I don't mind waiting until you have time for this topic, @nukeop.

IMO this is a low hanging fruit for improving all metadata providers and consolidating the code..
The only possible issue I see here is the possibly increased usage of the Spotify API.

[nukeop]

Thanks, I need to dedicate some focused time to checking this out. I'm clearing my backlog successively and will get back to it today or tomorrow, same as with your other PR.

[nukeop]

Ok, I looked through the code and here are my comments:

• I wouldn't call it a factory. A factory as a pattern creates objects according to a set of parameters. This class is more of a repository, or a service.
• The last.fm operations are pretty well tested. It's a bunch of unit tests, so they could be replaced with integration tests.
• I'm fine with this code living outside the Discogs provider, and you're right that it's a good idea to reuse it elsewhere. It reduces complexity. But given the low usage (and usability) of Musicbrainz provider, I'm rethinking if we actually need different metadata providers at all.
• Different similar artist repositories could be injected into metadata providers at runtime as strategies, with your creation as the first of its kind. You'd need to define a common interface that abstracts away all the last.fm implementation details. And then we could have different repositories that do this.

[sferra]

I wouldn't call it a factory..

I was pretty sure that the name wouldn't remain like that. The class creates SimilarArtist instances and assigns thumbnails (or not), so I went with "factory" for a start. We can rename it to whatever you see fit.

The last.fm operations are pretty well tested. It's a bunch of unit tests, so they could be replaced with integration tests.

I wouldn't discard one type of test in favor of another. IMO it's better to just add additional tests.

[...] I'm rethinking if we actually need different metadata providers at all.

I personally switch between Discogs and Bandcamp for metadata as they provide mutually exclusive information in some cases. Some artist (specifically independent, not-very-commercial ones) can't be found on Discogs or even have Bandcamp specific releases. The other way around, many popular artists don't bother with Bandcamp. I personally don't care much about the other metadata providers.

Different similar artist repositories could be injected [...] as strategies

Is a strategy really a strategy if there are no alternatives? Where would the choice for the strategy be made?
While abstracting the resolution of similar artists from LastFm is a very good idea, we don't (yet) have an alternative for that type of information. Also the LastFm metadata is also used for other purposes in all providers.

I'll try to come up with a constructive way for abstracting and reusing the LastFM metadata.
IMO it's best to first isolate and reuse functionality. Afterwards it's easier to detect and create abstractions like the one you suggested.

[nukeop]

Is a strategy really a strategy if there are no alternatives? Where would the choice for the strategy be made?

It could be a selectable plugin. Conceptually there is som overlap between this and the functionality that provides the autoradio feature. I experimented with having other providers there as well, but maybe most users don't care about that and just want to see a nice populated list.

I'll try to come up with a constructive way for abstracting and reusing the LastFM metadata.
IMO it's best to first isolate and reuse functionality. Afterwards it's easier to detect and create abstractions like the one you suggested.

Ok, I agree and I'd like to see where you end up with this. Take as much time as you need.

[sferra]

Seeing as to how this topic takes so much time to move forward (my fault, no time), I suggest splitting the topics (fix, refactoring and conceptual discussion):

• provide a fix here so
  • artist images are available for all providers
  • the changes in the PR don't become stale and can be still merged
• create a separate issue to discuss the concept of a "similar artist" plugin (or handling lastfm metadata) - i'm not sure
• create a separate issue to discuss your doubts about the necessity of metadata providers (if you think it't worth)

Having things separate would at least allow us to move some of these aspects forward while others are still being clarified. What do you think?

[sferra]

I'm sorry but I currently don't have the time to work on the enhancement regarding the LastFM metadata. Maybe I'll pick it up in the future.

[nukeop]

No problem, we could close this PR for the time being, and then you can reopen it when you have more time. Is that ok?

[sferra]

My suggestion would be to take the fix for now, but anything else is also alright for me.

Please close or merge the PR, as you wish

[nukeop]

Ok, I'll merge what we've got now, thanks for contributing.