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

Dumping improvement ideas for spotipy version 3 #652

Open
stephanebruckert opened this issue Mar 4, 2021 · 18 comments
Open

Dumping improvement ideas for spotipy version 3 #652

stephanebruckert opened this issue Mar 4, 2021 · 18 comments

Comments

@stephanebruckert
Copy link
Member

stephanebruckert commented Mar 4, 2021

Let's start thinking of the next major version for spotipy, while we keep maintaining the version 2.

spotipy's philosophy for v3 remains the same:

A light weight Python library for the Spotify Web API

Light weight means we keep it simple and let developers parse the JSON results themselves, as we trust that the Spotify API is backward-compatible.

v3 PRs and issues can use the label:

To do in version 3:

Feel free to suggest anything that you believe could improve spotipy in the long term. Independent issues / PRs can still be created and referenced in this discussion.

Development process

Here is a first suggestion, and can be improved:

  • a new v3 branch exists, so v3 PRs are welcome now!
  • all backward-incompatible PRs can be merged into v3
  • backward-compatible PRs can be merged into master as usual
  • PRs welcome to regularly rebase master onto v3 while making sure it doesn't include any of the things we don't want in v3
  • we will be able to create pre-releases along the way https://www.python.org/dev/peps/pep-0440/#id27

There is no rush, this will take the time it needs. Any suggestion is welcome! Thank you

@Peter-Schorn
Copy link
Contributor

I've noticed that the initializer for Spotify has three parameters that all alias each other: client_credentials_manager, oauth_manager, and auth_manager. We should remove the first two because they just create confusion.

@Peter-Schorn
Copy link
Contributor

Peter-Schorn commented Mar 12, 2021

DONE √ in #665


Here's another idea: How about creating an Enum for all of the authorization scopes:

from enum import Enum
from typing import Iterable
import re

class Scope(Enum):
    ugc_image_upload = "ugc-image-upload"
    user_read_recently_played = "user-read-recently-played"
    user_read_playback_state = "user-read-playback-state"
    playlist_modify_public = "playlist-modify-public"
    # and so on...

    @staticmethod
    def make_string(scopes: Iterable['Scope']) -> str:
        """
        Converts a sequence of scopes to a string.

        * scopes: An iterable of scopes.

        returns: a space-separated string of scopes
        """
        return " ".join([scope.value for scope in scopes])

    @staticmethod
    def from_string(scope_string: str) -> set['Scope']:
        """
        Converts a string of (usuallly space-separated) scopes into a
        set of scopes

        * scope_string: a string of scopes

        returns: a set of scopes.
        """
        scope_string_list = re.split(pattern=r"[\W-]+", string=scope_string)
        scopes = set()
        for scope_string in scope_string_list:
            try:
                scope = Scope(scope_string)
                scopes.add(scope)
            except ValueError as error:
                pass
        return scopes

scopes: set[Scope] = {
    Scope.ugc_image_upload,
    Scope.user_read_playback_state,
    Scope.playlist_modify_public
}

scope_string = Scope.make_string(scopes)
print(scope_string)

converted_scopes = Scope.from_string(scope_string)
print(converted_scopes)

This can be a purely additive change to v3. We can still accept the scopes as a string string or a list of strings.

I see several advantages to an enum:

  • It prevents typos
  • It enables autocomplete by your IDE, which not only makes the scopes faster to type, but enables better discoverability of the available scopes

stephanebruckert added a commit that referenced this issue Apr 10, 2021
* Added an exception clause that catches `FileNotFoundError` and logs a debug message in `SpotifyOAuth.get_cached_token`, `SpotifyPKCE.get_cached_token` and `SpotifyImplicitGrant.get_cached_token`.

* Changed docs for `auth` parameter of `Spotify.init` to `access token` instead of `authorization token`. In issue #599, a user confused the access token with the authorization code.

* Updated CHANGELOG.md

* Removed `FileNotFoundError` because it does not exist in python 2.7 (*sigh*) and replaced it with a call to `os.path.exists`.

* Replaced ` os.path.exists` with `error.errno == errno.ENOENT` to supress errors when the cache file does not exist.

* Changed docs for `search` to mention that you can provide multiple multiple types to search for. The query parameters of requests are now logged. Added log messages for when the access token and refresh tokens are retrieved and when they are refreshed. Other small grammar fixes.

* Removed duplicate word "multiple" from CHANGELOG

* * Fixed the bugs in `SpotifyOAuth.refresh_access_token` and `SpotifyPKCE.refresh_access_token` which raised the incorrect exception upon receiving an error response from the server. This addresses #645.
* Fixed a bug in `RequestHandler.do_GET` in which the non-existent `state` attribute of  `SpotifyOauthError` is accessed. This bug occurs when the user clicks "cancel" in the permissions dialog that opens in the browser.
* Cleaned up the documentation for `SpotifyClientCredentials.__init__`, `SpotifyOAuth.__init__`, and `SpotifyPKCE.__init__`.

* Removed unneeded import

* Added cache handler to `SpotifyClientCredentials` and fixed a bug in refresh tokens methods that raised the wrong exception (#655)

* Added an exception clause that catches `FileNotFoundError` and logs a debug message in `SpotifyOAuth.get_cached_token`, `SpotifyPKCE.get_cached_token` and `SpotifyImplicitGrant.get_cached_token`.

* Changed docs for `auth` parameter of `Spotify.init` to `access token` instead of `authorization token`. In issue #599, a user confused the access token with the authorization code.

* Updated CHANGELOG.md

* Removed `FileNotFoundError` because it does not exist in python 2.7 (*sigh*) and replaced it with a call to `os.path.exists`.

* Replaced ` os.path.exists` with `error.errno == errno.ENOENT` to supress errors when the cache file does not exist.

* Changed docs for `search` to mention that you can provide multiple multiple types to search for. The query parameters of requests are now logged. Added log messages for when the access token and refresh tokens are retrieved and when they are refreshed. Other small grammar fixes.

* Removed duplicate word "multiple" from CHANGELOG

* * Fixed the bugs in `SpotifyOAuth.refresh_access_token` and `SpotifyPKCE.refresh_access_token` which raised the incorrect exception upon receiving an error response from the server. This addresses #645.
* Fixed a bug in `RequestHandler.do_GET` in which the non-existent `state` attribute of  `SpotifyOauthError` is accessed. This bug occurs when the user clicks "cancel" in the permissions dialog that opens in the browser.
* Cleaned up the documentation for `SpotifyClientCredentials.__init__`, `SpotifyOAuth.__init__`, and `SpotifyPKCE.__init__`.

* Removed unneeded import

Co-authored-by: Stéphane Bruckert <stephane.bruckert@gmail.com>

* Made `CacheHandler` an abstract base class

Added:

* `Scope` - An enum which contains all of the authorization scopes (see [here](#652 (comment))).

* Added the following endpoints
    * `Spotify.current_user_saved_episodes`
    * `Spotify.current_user_saved_episodes_add`
    * `Spotify.current_user_saved_episodes_delete`
    * `Spotify.current_user_saved_episodes_contains`
    * `Spotify.available_markets

* Fixed formatting issues. Removed python 2.7 from github workflows.

* Added python 3.9 to github workflows. The type hints for set now uses the generic typing.Set instead of builtins.set.

* Changed f-string to percent-formatted string.

* Fixed the duplicate "###Changed" section in the change log.

Co-authored-by: Stéphane Bruckert <stephane.bruckert@gmail.com>
@stephanebruckert
Copy link
Member Author

https://github.com/plamere/spotipy/blob/master/spotipy/cache_handler.py

As we add more and more cache handlers, we would need to split the cache handler file into single files and move them in a folder.

Suggestions:

  • cache_handlers/
    • base_cache_handler.py (added base_)
    • cache_file_handler.py
    • memory_cache_handler.py

or:

  • cache_handlers/
    • cache_handler.py
    • connectors/
      • cache_file_handler.py
      • memory_cache_handler.py

or:

  • cache_handler.py
  • cache_handlers/
    • cache_file_handler.py
    • memory_cache_handler.py

@Peter-Schorn
Copy link
Contributor

I like the first layout the most; it's the simplest. I suggest we use the same structure for the auth managers as well.

@mmattbtw
Copy link

mmattbtw commented Aug 1, 2021

Maybe add event handlers for when someone skips/plays the next song.

@Peter-Schorn
Copy link
Contributor

Peter-Schorn commented Aug 1, 2021

That's not possible with the web API. All of the endpoints are documented here and spotipy already has a method for every endpoint and every authorization method.

@nleroy917
Copy link
Contributor

Headless authentication has always seemed to be a hot-ticket item

@IdmFoundInHim
Copy link
Contributor

IdmFoundInHim commented Aug 10, 2021

I think that some sort of handling/abstraction for Spotify's paging objects should be included because many of the basic endpoint return these paging objects. My guess is that most projects depending on spotipy have to deal with these, which requires making manual HTTP requests. This lies on the border of "let developers parse JSON results themselves" and the Spotify API itself; The object has to be parsed to deduce if another request is needed, but to make that request you need the auth token. If spotipy is supposed to work out of the box, this feature is necessary for many basic endpoints. A dependent package otherwise must import requests and do the extra work of integrating spotipy functions with those requests.

In my experience, the best way to deal with these is to convert them to a more Pythonic iterable — namely, a generator. Because it uses lazy evaluation, it keeps the memory advantages of a paging object yet can be easily converted to a list if resources are available in excess.

The most aggressive solution would be to modify the return values of endpoints that include paging objects (e.g. Spotify.playlist_items), replacing each JSON object that has a 'next' attribute with a generator. This would create a full abstraction layer, making it easy for someone unfamiliar with the API to use spotipy. However, it eliminates the convenient 'total' attribute and results in a dict that cannot be trivially converted back to portable JSON. Alternatively, we could include this functionality as an option in relevant endpoints (as_generator=True or something similar). That could be done in spotipy 2 (default is paging object) or 3 (default is generator).

Another solution, which may be fully backwards compatible, is adding a function to the Spotify class or the auth manager classes. This function would take a paging object and return a generator. I have a specialized implementation of this in my own project, and could enhance it for inclusion if this is the desired solution.

@IdmFoundInHim
Copy link
Contributor

For the endpoints that limit input to 50 or 100 items, spotipy should accept any amount of input, breaking it up into smaller requests internally. Currently, devs using spotipy have to implement this themselves if their programs have any chance of exceeding the limit.

@nleroy917
Copy link
Contributor

@IdmFoundInHim I agree that the paging aspect of the API can be a bit of a nuisance... but I feel like the .next() method of spotipy already makes this so easy. This is some code I use regularly to iterate over paged objects:

all_playlists = []
playlists = spotipy.current_user_playlists(limit=50)
all_playlists += playlists['items']
        
while playlists['next']:
    playlists = spotipy.next(playlists)
    all_playlists += playlists['items']

# do something with playlists

Maybe on methods/functions that can potentially lead to pagination, add a page_items=True flag and it conducts the above code for you?

I also agree an internal "grouping algorithm," would be great since some endpoints only accept a certain number of ids. I always have to write this myself in order to achieve seamless use.

@Peter-Schorn
Copy link
Contributor

Perhaps we should have an extend method which, similar to next, accepts a page of results and then requests all additional pages.

@nleroy917
Copy link
Contributor

Wouldn't that pretty much render next obsolete? I feel like most would just opt for extend. Maybe you could add a flag to the next method:

all_playlists = []
page = spotipy.current_user_playlists(limit=50)
all_playlists += page['items']
if page['next']:
    playlists += spotipy.next(page['next'], all_pages=True)['items']

I think here you prevent the user from accessing other data in that page object. I guess, however, that is their choice if they elect to use an extend method or an extend=True flag.

... if I understand correctly here.

@Peter-Schorn
Copy link
Contributor

Peter-Schorn commented Aug 18, 2021

Some people need fine-grained control over when and which pages to retrieve (think of an infinitely-scrolling list), so next should not be rendered obsolete if we add the extend method. Further, the word next implies that only a single additional page will be retrieved, so adding an all_pages parameter to it seems weird. Instead, it should be a separate method:

def extend(self, page):
    results = page["items"]
    while page["next"]:
        page = self.next(page)
        results.extend(page["items"])
    return results

@IdmFoundInHim
Copy link
Contributor

I actually wasn't aware of Spotify.next, but shouldn't we be making the objects compatible with the Python builtins instead of duplicating them? It would be intuitive for next(Spotify.playlist_items(...)) to return the next playlist item, rather than a paging object.

Converting to an iterator would take care of both use cases (controlled or mass retrieval), and would avoid loading unnecessarily large objects into memory.

def page_iter(self, page):
	yield from page['items']
	while page['next']:
		page = self.next(page)
		yield from page['items']	

This would allow you to do:

playlists = spotify.page_iter(spotify.current_user_playlist(limit=50))
for playlist in playlists:
	# For example:	
	spotify.playlist_add_items(playlist['id'], [your_favorite_song])

...as well as...

liked_songs = spotify.page_iter(spotify.current_user_saved_songs())

def on_scroll_end():
	render_songs(itertools.islice(liked_songs, 20))

stephanebruckert added a commit that referenced this issue Dec 26, 2022
* Added an exception clause that catches `FileNotFoundError` and logs a debug message in `SpotifyOAuth.get_cached_token`, `SpotifyPKCE.get_cached_token` and `SpotifyImplicitGrant.get_cached_token`.

* Changed docs for `auth` parameter of `Spotify.init` to `access token` instead of `authorization token`. In issue #599, a user confused the access token with the authorization code.

* Updated CHANGELOG.md

* Removed `FileNotFoundError` because it does not exist in python 2.7 (*sigh*) and replaced it with a call to `os.path.exists`.

* Replaced ` os.path.exists` with `error.errno == errno.ENOENT` to supress errors when the cache file does not exist.

* Changed docs for `search` to mention that you can provide multiple multiple types to search for. The query parameters of requests are now logged. Added log messages for when the access token and refresh tokens are retrieved and when they are refreshed. Other small grammar fixes.

* Removed duplicate word "multiple" from CHANGELOG

* * Fixed the bugs in `SpotifyOAuth.refresh_access_token` and `SpotifyPKCE.refresh_access_token` which raised the incorrect exception upon receiving an error response from the server. This addresses #645.
* Fixed a bug in `RequestHandler.do_GET` in which the non-existent `state` attribute of  `SpotifyOauthError` is accessed. This bug occurs when the user clicks "cancel" in the permissions dialog that opens in the browser.
* Cleaned up the documentation for `SpotifyClientCredentials.__init__`, `SpotifyOAuth.__init__`, and `SpotifyPKCE.__init__`.

* Removed unneeded import

* Added cache handler to `SpotifyClientCredentials` and fixed a bug in refresh tokens methods that raised the wrong exception (#655)

* Added an exception clause that catches `FileNotFoundError` and logs a debug message in `SpotifyOAuth.get_cached_token`, `SpotifyPKCE.get_cached_token` and `SpotifyImplicitGrant.get_cached_token`.

* Changed docs for `auth` parameter of `Spotify.init` to `access token` instead of `authorization token`. In issue #599, a user confused the access token with the authorization code.

* Updated CHANGELOG.md

* Removed `FileNotFoundError` because it does not exist in python 2.7 (*sigh*) and replaced it with a call to `os.path.exists`.

* Replaced ` os.path.exists` with `error.errno == errno.ENOENT` to supress errors when the cache file does not exist.

* Changed docs for `search` to mention that you can provide multiple multiple types to search for. The query parameters of requests are now logged. Added log messages for when the access token and refresh tokens are retrieved and when they are refreshed. Other small grammar fixes.

* Removed duplicate word "multiple" from CHANGELOG

* * Fixed the bugs in `SpotifyOAuth.refresh_access_token` and `SpotifyPKCE.refresh_access_token` which raised the incorrect exception upon receiving an error response from the server. This addresses #645.
* Fixed a bug in `RequestHandler.do_GET` in which the non-existent `state` attribute of  `SpotifyOauthError` is accessed. This bug occurs when the user clicks "cancel" in the permissions dialog that opens in the browser.
* Cleaned up the documentation for `SpotifyClientCredentials.__init__`, `SpotifyOAuth.__init__`, and `SpotifyPKCE.__init__`.

* Removed unneeded import

Co-authored-by: Stéphane Bruckert <stephane.bruckert@gmail.com>

* Made `CacheHandler` an abstract base class

Added:

* `Scope` - An enum which contains all of the authorization scopes (see [here](#652 (comment))).

* Added the following endpoints
    * `Spotify.current_user_saved_episodes`
    * `Spotify.current_user_saved_episodes_add`
    * `Spotify.current_user_saved_episodes_delete`
    * `Spotify.current_user_saved_episodes_contains`
    * `Spotify.available_markets

* Fixed formatting issues. Removed python 2.7 from github workflows.

* Added python 3.9 to github workflows. The type hints for set now uses the generic typing.Set instead of builtins.set.

* Changed f-string to percent-formatted string.

* Fixed the duplicate "###Changed" section in the change log.

Co-authored-by: Stéphane Bruckert <stephane.bruckert@gmail.com>
stephanebruckert referenced this issue Jan 24, 2023
* Improve URL and URI handling

* Back to SpotifyException for backward-compatibility

* fix copy paste typo

* TODO v3 comments

Co-authored-by: Stephane Bruckert <stephane.bruckert@gmail.com>
@timrae
Copy link

timrae commented Apr 5, 2023

Use aiohttp and enable Async/await in the spotipy interface

HighnessAtharva added a commit to HighnessAtharva/spotipy that referenced this issue Apr 8, 2023
Implemented suggestions for spotipy-dev#652
Major changes include:-
- Using if expressions than if conditions
list comprehensions instead of a for loop for smaller logical blocks.
- Usage of f-strings ( I saw that the env supports 3.4 but f-strings have been added from Python 3.6). So should I refactor this?
- Simplifying if/else code blocks to be more concise.
- Merge nested conditional clauses.
@hansmbakker
Copy link

There is a request for allowing the Device Authorization Grant flow on the Spotify forum - it is very useful for embedded usecases like a remote control.

@Ousret
Copy link

Ousret commented Dec 11, 2023

Hello there,

You may be interested in https://github.com/jawah/niquests for a transparent upgrade of Requests.
Have async, sync, and multiplexed connection. Support HTTP/2, and 3.

@wallysaurus
Copy link

That's not possible with the web API. All of the endpoints are documented here and spotipy already has a method for every endpoint and every authorization method.

has this been updated since? I'm seeing this page on the official website now, although it's in javascript.
https://developer.spotify.com/documentation/web-playback-sdk/reference#events

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants