Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Deezer plugin #3355

Merged
merged 41 commits into from
Sep 14, 2019
Merged

Add Deezer plugin #3355

merged 41 commits into from
Sep 14, 2019

Conversation

rhlahuja
Copy link
Contributor

@rhlahuja rhlahuja commented Sep 2, 2019

Add a Deezer plugin to allow tracks and albums to be matched using the Deezer API.

Introduce a base MetadataSourcePlugin class to define shared API import logic. This will be integrated into the Spotify plugin in a followup PR.

@sampsyo
Copy link
Member

sampsyo commented Sep 3, 2019

Awesome! Looking pretty good so far.

I have just a couple of high-level questions:

  • Since you said you repurposed some logic from your Spotify plugin, would it make sense to factor out some code that gets reused into a common library?
  • It looks like there are also changes to the Spotify plugin here too—would you mind breaking those out into a separate PR?

@rhlahuja
Copy link
Contributor Author

rhlahuja commented Sep 3, 2019

  • Yeah, I was going back and forth on this. Some helpers like album_distance, track_distance, and _get_artist can easily be factored out, although some of the higher-level functions like track_for_id and album_for_id may not quite be similar enough to easily modularize. I'll take another look at this later and refactor what makes sense. Is there a particular place you'd like to place the common library? Something like beetsplug/api_utils.py?
  • Sure thing -- I'll include the aforementioned Spotify refactor in a separate PR.

@sampsyo
Copy link
Member

sampsyo commented Sep 3, 2019

Cool; thanks for looking into it! Depending on the exact scope, something like beets.autotag or beets.autotag.match could be a good fit for these reusable bits.

@rhlahuja
Copy link
Contributor Author

rhlahuja commented Sep 5, 2019

@sampsyo I've factored out shared logic into a base APIAutotaggerPlugin class (better name suggestions welcome). I plan to revert all changes to spotify.py and discogs.py and include them in a separate PR, but I committed them all here for now to get a sense of the final state. Before I begin testing and documenting, what are your thoughts so far?

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome; thank you for working on this! This looks great overall. I left a few comments inline.



@six.add_metaclass(abc.ABCMeta)
class APIAutotaggerPlugin(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, perhaps the most natural home for this base class would be in the beets.plugins module?

In general, we have talked several times in beets's history about creating "template" base classes like this one to make it simpler to write certain kinds of plugins. So I'm very happy to see that you came to the same idea independently! It would be great to introduce more of these, so perhaps creating a new place to gather plugin templates together would be the thing to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what do you think about the name MetadataSourcePlugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I was unsure if this class is the right approach, but it's great to hear something like this was already being considered. And I agree, beets.plugins.MetadataSourcePlugin seems like a more natural place/name for this.

* :doc:`/plugins/deezer`:
* Added Deezer plugin as an import metadata provider: you can now match tracks
and albums using the `Deezer`_ database.
Thanks to :user:`rhlahuja`.

Fixes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even mention the new base class in a "for developers" section.

* :doc:`/plugins/deezer`:
* Added Deezer plugin as an import metadata provider: you can now match tracks
and albums using the `Deezer`_ database.
Thanks to :user:`rhlahuja`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a nested list here if it's just one bullet.

--------------------

* You're a Beets user.
* You want to autotag music with metadata from the Deezer API.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section doesn't seem to add too much—both of these points are pretty well covered by your introductory sentence above. 😃

@rhlahuja
Copy link
Contributor Author

@sampsyo this is tested and ready for a final review. Once merged, I'll open another PR for reusing this shared logic in the Spotify and Discogs plugins.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This looks great! I have one small comment above, but I can resolve it myself during the merge. Thank you for taking the time to make this base class awesome! ✨

AlbumMatch,
TrackMatch,
Distance,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change can be reverted now because Distance is no longer used here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops; I'm obviously wrong about that. Not changing anything!

sampsyo added a commit that referenced this pull request Sep 14, 2019
sampsyo added a commit that referenced this pull request Sep 14, 2019
@sampsyo sampsyo merged commit 0b2837d into beetbox:master Sep 14, 2019
@rhlahuja rhlahuja mentioned this pull request Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants