-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Deezer plugin #3355
Conversation
Awesome! Looking pretty good so far. I have just a couple of high-level questions:
|
|
Cool; thanks for looking into it! Depending on the exact scope, something like |
@sampsyo I've factored out shared logic into a base |
There was a problem hiding this 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.
beets/autotag/__init__.py
Outdated
|
||
|
||
@six.add_metaclass(abc.ABCMeta) | ||
class APIAutotaggerPlugin(object): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
docs/changelog.rst
Outdated
* :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`. |
There was a problem hiding this comment.
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.
docs/plugins/deezer.rst
Outdated
-------------------- | ||
|
||
* You're a Beets user. | ||
* You want to autotag music with metadata from the Deezer API. |
There was a problem hiding this comment.
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. 😃
@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. |
There was a problem hiding this 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, | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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.