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 Spotify OAuth support and autotagger integration #3123

Merged
merged 34 commits into from Jan 21, 2019

Conversation

Projects
None yet
2 participants
@rhlahuja
Copy link
Contributor

commented Jan 20, 2019

Addressing #2694, I've added OAuth support for the Spotify plugin using Client Credentials Flow. This requires a registered application and Client ID/Secret to be provided via client_id and client_secret under the spotify options in config.yaml.

I've also started working on adding Spotify support for the importer (incomplete album_for_id and track_for_id funcs included in this PR with some logic copied from the Discogs plugin), since I've come across more than a few releases that don't exist in MusicBrainz/Discogs/Beatport/Bandcamp but do in Spotify. However, I realize that the Spotify API doesn't provide certain fields like album media, catalognum, etc. like these other beets sources do. So, @sampsyo, do you think it's still worth extending the autotagger with Spotify matches, despite offering incomplete metadata?

rhlahuja added some commits Jan 20, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Awesome; this is a great start!

We can register for a proper API key. For the tests, however, we should avoid needing to communicate with the actual Spotify API server—instead, the tests should mock the API’s responses so they can be run even without a connection to the Internet.

@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

@sampsyo
Copy link
Member

left a comment

Very nice! This is looking good. I had only a couple of small style comments after a review. Could you please also look into documenting the new functionality for the plugin?

Show resolved Hide resolved beetsplug/spotify.py Outdated
Show resolved Hide resolved beetsplug/spotify.py Outdated
Show resolved Hide resolved beetsplug/spotify.py Outdated
@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

I've added some documentation to spotify.rst regarding the new functionality, and happy to add more if necessary.

One thing I'm not quite sure about is the proper implementation of the album_distance and track_distance functions. For now, I've simply copied these from the Beatport plugin, but let me know if we need to add anything else. I've also specified the Spotify ID for AlbumInfo.album_id and TrackInfo.track_id values which I think is correct behavior, but would like to confirm.

Thanks again for the swift replies all your work on this awesome project!

Edit: typo

@rhlahuja rhlahuja changed the title Add Spotify OAuth support Add Spotify OAuth support and autotagger integration Jan 20, 2019

rhlahuja added some commits Jan 20, 2019

@sampsyo
Copy link
Member

left a comment

Nice! I have just a few writing suggestions for improving the docs. Other than this, do you think this is ready to merge?

@@ -67,10 +106,20 @@ in config.yaml under the ``spotify:`` section:
track/album/artist fields before sending them to Spotify. Can be useful for
changing certain abbreviations, like ft. -> feat. See the examples below.
Default: None.
- **tokenfile**: Filename of the JSON file stored in the beets configuration
directory to use for caching the OAuth access token.
Default: ``spotify_token.json``.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Jan 20, 2019

Member

Hmm… perhaps we shouldn't even mention this option in the docs. I don't expect anyone will ever need to change it, and omitting the unnecessary options should make the docs page a little easier to read. (FWIW, we don't document tokenfile for the other OAuth-based plugins we have.)

* Back Seat Driver (Spirit Guide) -> Back Seat Driver (Spirit Guide) (source)
* 2AM -> 2AM (source)
[A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates, plaY?

This comment has been minimized.

Copy link
@sampsyo

sampsyo Jan 20, 2019

Member

I'm not sure having the whole candidate display is necessary here. Maybe the just the first two lines would be enough to get the idea across?

Show resolved Hide resolved docs/plugins/spotify.rst
library with the ``beet spotify`` command. Using the `Spotify Search API`_,
any tracks that can be matched with a Spotify ID are returned, and the
results can be either pasted in to a playlist or opened directly in the
Spotify app.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Jan 20, 2019

Member

Here's a slightly more direct way to say this without the passive voice: "Also, the plugin can use the Spotify Search API to provide metadata matches for the importer."

This comment has been minimized.

Copy link
@rhlahuja

rhlahuja Jan 20, 2019

Author Contributor

This part of the doc is actually referring to the beet spotify command which lists Spotify URLs for existing library tracks, but good point about using the active voice. I've consolidated this with the portion below and it now reads:

The spotify plugin generates Spotify playlists from tracks in your
library with the beet spotify command using the Spotify Search API.
Also, the plugin can use the Spotify Album and Track APIs to provide
metadata matches for the importer.

Not sure if we even need to specifically mention that we're using the Search/Album/Track APIs, I can remove that if not.


Spotify URLs and IDs may also be provided to the ``Enter release ID:`` prompt
during ``beet import`` to autotag music with data from the Spotify
`Album`_ and `Track`_ APIs.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Jan 20, 2019

Member

We can probably get away without mentioning this here—the "search by ID" functionality is a standard part of any plugin that extends the importer with a metadata backend, so the above description (and the detail below) probably covers this just fine.

@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

This should be ready to merge assuming album_distance and track_distance are fine as-is. I've also addressed an edge case with TrackInfo.index not being set if only a single track is being looked up / imported (need to query the Album API and enumerate through tracks to find its position on the entire release, since the Track API only provides medium_index).

rhlahuja added some commits Jan 20, 2019

@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2019

Actually, I should also add some logic to populate TrackInfo.medium_total. Let's hold off on merging until I can get to that later today.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Looking good! Just one last thing: do you want to include that API key in the default configuration? If so, perhaps we should mention that it's actually optional to register for your own Spotify app; you can just use the included key unless you really want to change it. If not, then maybe we should not provide a default to force people to register with Spotify themselves.

@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Yea, that client ID/secret isn't actually valid (just randomly generated strings), so I agree we should either register a dedicated Spotify app and include those creds or remove them entirely and force people to register one themselves. I noticed the Discogs plugin sets a default client ID/secret which I'm assuming is for a dedicated app, so it seems like that's the preferred approach? Not sure if we would run into issues with rate limiting or anything else with many users sharing the same app. Which option do you prefer?

rhlahuja added some commits Jan 21, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Sounds good. Let's start by offering an "official" app key and revisit (i.e., reset the key and force people to register for their own apps) if it becomes a problem. Here are credentials for an app I just registered:

  • client ID: 4e414367a1d14c75a5c5129a627fcab8
  • client secret: f82bdc09b2254f1a8286815d02fd46dc
@rhlahuja

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Awesome, I changed the default to those and removed all mentions of the Spotify app from the docs (or should we still include that it's optional?). This should be ready to merge now if everything else looks good.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Looks perfect! I'll merge this now.

@sampsyo sampsyo merged commit dab62f2 into beetbox:master Jan 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Jan 21, 2019

Merge pull request #3123 from rhlahuja/spotify-oauth
Add Spotify OAuth support and autotagger integration

sampsyo added a commit that referenced this pull request Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.