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

Refactor client internal calls to allow keepalive #269

Merged

Conversation

sonneveld
Copy link
Contributor

Internal calls were closing the http connection after every request, which meant that subsequent queries had to recreate the connection. By only closing the request object, I halved the time it took to get my saved tracks:

loading saved tracks before:
real 0m39.799s

loading saved tracks after:
real 0m19.340s

c0de-fox pushed a commit to c0de-archive/spotipy that referenced this pull request Apr 24, 2018
This merges [PR-269](spotipy-dev#269) from
the base spotipy project ahead of it getting merged in upstream
c0de-fox pushed a commit to c0de-archive/spotipy that referenced this pull request Apr 24, 2018
This merges [PR-269](spotipy-dev#269) from
the base spotipy project ahead of it getting merged in upstream.
@Harrison97
Copy link
Contributor

Hey! I am hosting a new master branch for this repo since the old creator has been MIA for 2 years. Hoping to get this repo back on it's feet and in good standing. I've been working on updating tests and adding all new functionality and have merged some of the PR's that I thought were sufficient for merging. If your PR didn't make it, resubmit it please.

Thanks!

https://github.com/Harrison97/spotipy

@stephanebruckert
Copy link
Member

stephanebruckert commented Jan 13, 2020

Hey @sonneveld. Great PR, thanks! Just ran the tests locally, and integration tests are twice as fast:

----------------------------------------------------------------------
Ran 60 tests in 8.069s

OK

vs.

----------------------------------------------------------------------
Ran 60 tests in 20.057s

OK

Going to resolve conflicts and merge it. Will open a new one to fix a warning in the tests

@stephanebruckert
Copy link
Member

TIL we are allowed to push on a fork's branch if a PR is open:

You can commit changes on a pull request branch that was created from a fork of your repository with permission from the pull request creator.

https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

@sonneveld
Copy link
Contributor Author

Nice! Just caught up on the news of the new maintainers. Good luck and thanks for taking it on.

Copy link
Collaborator

@ritiek ritiek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @sonneveld! Looks good to me.

This example code now consistently runs considerably faster:

playlist_url = "https://open.spotify.com/playlist/58mpINjST18mK6qvvPYRzO"
playlist = sp.playlist(playlist_url)

tracks = playlist["tracks"]
while True:
    for item in tracks["items"]:
        print(item["track"]["name"])
    if not tracks["next"]:
        break
    tracks = sp.next(tracks)

Takes ~26s without this PR and ~15s with this PR.

@stephanebruckert stephanebruckert merged commit c2b6be6 into spotipy-dev:master Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants