Skip to content

added paginator to spotify client #506

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fcusson
Copy link

@fcusson fcusson commented Feb 8, 2025

Proposed Changes

Implementation of a page read that is able to go through limit and offset logic api endpoint to retrieve all or a set number of items from the API. This function will enable to centralize the logic of retrive whole list of items (playlists, album tracks, etc.) from the API without requiring to implement the same logic in multiple methods

This functionality is required for Spotcast to be able to move to spotifyaio

pytest has been validated to provide full coverage with current process.

@fcusson
Copy link
Author

fcusson commented Feb 11, 2025

Hi @joostlek,

just wanted to point out I made a PR for spotifyaio and also provide some details regarding the change.

Currently, this PR is purely foundation implementation for requirements that Spotcast has. We need to be able to pull an arbitrary number of items from the API that sometimes require more than the limit per single call. The _paginator method will help as a private method to be used by other functions to be able to manage the logic of limit and offset to pull the required amount of items.

The method can be called with the parameter max_items to any int, in this case, it will pull up to that amount of items and return them, or it can be called with None where the method will infer the total number of items from the result (which contains a total key) and pull everything.

This is refactored version of the _paginator Spotcast currently uses, but I do think it would be worth while for the underlying library to manage this part instead.

My goal after would be to refactor the different methods that have a limit and offset logic to use the _paginator instead.

If you have any questions on my change, please ask. Also, if there are any changes you would like to better fit in the structure and style of the library, please provide your comments.

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale There has not been activity on this issue or PR for quite some time. label Mar 14, 2025
@joostlek
Copy link
Owner

Would it maybe make sense to have a value return an async generator? This way we can just do

async for track in await client.get_tracks()
    pass

The twitch API does that and it makes the library really nice to work with, wdyt?

@fcusson
Copy link
Author

fcusson commented Mar 14, 2025

@joostlek, I like the idea, do you have a link for where its being used?

I'll look into making the change. I could replace the page reader into a generic function that handles the generator and then we would move the logic of stopping the read at the functions levels based on the max items requested. I'll review my current PR next week and get back to you.

@github-actions github-actions bot removed the stale There has not been activity on this issue or PR for quite some time. label Mar 15, 2025
@fcusson
Copy link
Author

fcusson commented Mar 21, 2025

@joostlek, was that what you had in mind? Modified the _paginator method to provide an async Generator instead. This will ensure the bulk of the offset and limit logic doesn't have to be replicated for each methods down the line, after, we can simply use the paginator in the following way:

async def get_tracks(self, max_items) -> Generator[dict, None, None]:
    async for chunk in self._paginator("endpoint", max_items=max_items):
        for track in chunk:
            yield track

Please tell me if you had something else in mind or would want deeper changes

@joostlek
Copy link
Owner

I have this implemented with aioyoutube, https://github.com/joostlek/python-youtube/blob/main/src/youtubeaio/youtube.py#L194-L212 for example. You can use a helper like first() or limit() to limit responses.

I got this from twitchAPI and I kinda like the way as it is easier for the implementer to actually use and don't care about how pages work. WDYT?

@fcusson
Copy link
Author

fcusson commented Apr 17, 2025

@joostlek. I like the idea and I'm all in to align on other projects. I'll try to make the modification during the weekend and get back to you on that. Thank you for the reference, that makes it much clearer

@joostlek
Copy link
Owner

I'll try to get to it quicker this time 👍🏻

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