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

saved tracks #35

Merged
merged 3 commits into from Jan 18, 2021
Merged

saved tracks #35

merged 3 commits into from Jan 18, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 13, 2021

added feature to analyze saved tracks and also fixed some bugs i found on the way.
This is still work in progress but I would appreciate some feedback on this.

Copy link
Owner

@nleroy917 nleroy917 left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you so much. I have been thinking about this on and off but wasn't sure how to implement it really. I like the idea of adding an endpoint for playlists - was probably needed anyway and clever to treat the saved tracks like a playlist.

Are you still working on this - should I wait to merge?

lib/Spotify.py Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 14, 2021

The conversion from saved tracks to playlist still needs a bit of work and I haven't tested yet if everything works properly. So don't merge this yet.

@nleroy917
Copy link
Owner

Sweet. I'll hold off - I pulled the changes and checked it out and it worked on my end. Let me know if you need help/make progress.

Implementeed requested changes
renamed id of saved tracks playlist to avoid possible confusion with
saved albums/shows
@ghost
Copy link
Author

ghost commented Jan 14, 2021

The conversion from saved tracks to a playlist object is now complete. I also cleaned up my code a bit and implemented your requested changes. So I you can merge this if everything is fine.

During testing I did come across an issue. When the playlist has only a few songs the lyrics fetching does not work when the page is reloaded in quick succession. This is related to this issue. There's nothing we can do about this except switching to a different lyrics api. I think this issue can be ignored for now.

Copy link
Owner

@nleroy917 nleroy917 left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you so much! Regarding that issue, I'm fine to ignore it for now ... that API is pretty hacky if I remember correctly, but It is faster than any other solution I could find and it isn't rate limited I believe... I can look into it more and see if I can't figure out why it's returning empty lyircs.

@nleroy917
Copy link
Owner

I'm going to go ahead and merge and deploy this

@nleroy917 nleroy917 closed this Jan 18, 2021
@nleroy917 nleroy917 reopened this Jan 18, 2021
@nleroy917 nleroy917 merged commit 8e2ed2c into nleroy917:master Jan 18, 2021
@nleroy917
Copy link
Owner

Deployed. I tested it on my Mac and it worked, but I also tested it on a windows machine I have and it was throwing a 500 error for insufficient client scope when pulling playlists. Worked on my mobile too.

I cleared the cache and cookies on the Windows machine and then it worked again - so somewhere the scope was being stored and it needed to be cleared to work again... Just an FYI

@ghost ghost deleted the saved_songs branch March 2, 2021 10:45
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.

1 participant