spotify playlist created #98
Conversation
parth-paradkar
requested review from
parth-paradkar,
ghostwriternr and
mukul-mehta
There was a problem hiding this comment.
This is a great initial PR, we can work more on top of this. Some suggestions:
- I think you should rename the directory to
spotify_playlist - We do not use the
inputfunction, since this code will run on the server. A playlist has already been created. You can add environment variables for Spotify playlist id, username, etc. Add them in the.envfile and change the.env.templateaccordingly. - You need to update the same playlist instead of creating a new one.
Thanks!
spotifyPlaylist/fetch_songs.py
Outdated
| songs = [] | ||
| q = session.query(Song).limit(50).all() | ||
| for i in q: | ||
| songs.append(i.name) |
There was a problem hiding this comment.
This can be replaced by a list comprehension to make it cleaner.
q = session.query(Song).limit(50).all()
return [song.name for song in q]
There was a problem hiding this comment.
Additionally, this function should be renamed to get_song_names
spotifyPlaylist/spotify_playlist.py
Outdated
|
|
||
|
|
||
| scope="playlist-modify-public" | ||
| username="Put your userid" |
There was a problem hiding this comment.
This should be loaded from an environment variable
spotifyPlaylist/spotify_playlist.py
Outdated
| playlist_name = input("Enter a playlist name: ") | ||
| playlist_description = input("Enter a playlist description: ") | ||
| spotifyObject.user_playlist_create(user=username,name=playlist_name,public=True,description=playlist_description) |
There was a problem hiding this comment.
We probably won't need this code once the playlist has been created. Instead, we should get the playlist from user playlists using its id, which can be loaded from the environment variables.
There was a problem hiding this comment.
Your code seems to assume that we create a new playlist every time. However, we need to update a single playlist with the latest songs.
spotifyPlaylist/spotify_playlist.py
Outdated
| for i in songs: | ||
| result = spotifyObject.search(q=i) | ||
| if(len(result['tracks']['items'])!=0): | ||
| list_of_songs.append(result['tracks']['items'][0]['uri']) |
There was a problem hiding this comment.
This can also be replaced by a list comprehension. Not so urgent though.
spotifyPlaylist/fetch_songs.py
Outdated
|
|
||
| engine = create_engine('Database URL') | ||
| engine.dialect.description_encoding = None | ||
| session = Session(bind=engine) |
There was a problem hiding this comment.
We are using a context manager that gets called to create sessions and commit the changes
You don't need to import sqlalchemy here since we have already written a context manager that handles this. Please have a look at c3po/db/metadata.py where it has been used.
|
@yashchau1303 please have a look at this where I have used the |
|
Hello @yashchau1303! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-12-18 13:27:12 UTC |
spotify_playlist/spotify_playlist.py
Outdated
| import os | ||
| import spotipy | ||
| from spotipy.oauth2 import SpotifyOAuth | ||
| import json |
There was a problem hiding this comment.
This import is not required here.
spotify_playlist/fetch_songs.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Please remove the whitespace
spotify_playlist/fetch_songs.py
Outdated
| engine = create_engine('postgresql+psycopg2://yash:root@localhost:5432/yash_test') | ||
| engine.dialect.description_encoding = None | ||
| session = Session(bind=engine) | ||
|
|
There was a problem hiding this comment.
We do not need this code. We are using context managers. If you need to use a session use with session_scope() as session: to create a new block. Import that from c3po.db.base. Refer to c3po/db/metadata.py for examples.
c3po/db/models/user.py
Outdated
| permalink_url = Column(String) | ||
| # permalink_url = Column(String) |
There was a problem hiding this comment.
Don't see how this is relevant to this PR. Can you please elaborate?
There was a problem hiding this comment.
I was getting an error as this column was not used further
spotify_playlist/spotify_playlist.py
Outdated
| playlist_id = os.getenv("SPOTIFY_PLAYLIST_ID") | ||
|
|
||
| # add songs to playlist | ||
| spotifyObject.user_playlist_add_tracks(user=username,playlist_id=playlist_id,tracks=list_of_songs) |
There was a problem hiding this comment.
We don't want to add all the songs that were fetched from the DB to be added again. For example, if we already have 50 songs in the playlist, this code will probably add 50 more on the second run, taking the number of songs to 100 with duplicates. Possible ways you can solve this:
- Remove all the songs in the playlist and the newly fetched songs.
- Write logic to cross-check between the list fetched from DB and the list of songs that already exist in the playlist and replace the ones that are not there in the list fetched from DB with the new ones.
There was a problem hiding this comment.
Please also keep in mind that you are using spotipy v2.13.0 that is installed as a dependency of metadata-extractor. Ensure that you are using the methods given in that version and not the latest version.
spotify_playlist/fetch_songs.py
Outdated
| # get first 50 songs from the db | ||
| def get_song_names(): | ||
| songs = [] | ||
| for u , l in session.query(UserPosts,Link).filter(UserPosts.link_id==Link.id).all(): |
There was a problem hiding this comment.
This is a heavy query that will consume a lot of time. It is basically fetching all the posts and links from the table, the count of which is huge. We do not want to run such a heavy DB query. Please make use of limit and offset and modify your code logic accordingly.
spotify_playlist/fetch_songs.py
Outdated
| session = Session(bind=engine) | ||
|
|
||
| # get first 50 songs from the db | ||
| def get_song_names(): |
There was a problem hiding this comment.
Change the name to get_song_url
| else: | ||
| continue |
There was a problem hiding this comment.
This is code is redundant
| songs = [] | ||
| for u,l in session.query(UserPosts,Link).filter(UserPosts.link_id==Link.id).limit(50).all(): | ||
| if len(songs)<=50: | ||
| if get_spotify_id(l.url) is not None: |
There was a problem hiding this comment.
This can be changed to simply if get_spotify_id(l.url):
| def get_song_url(): | ||
| with session_scope() as session: | ||
| songs = [] | ||
| for u,l in session.query(UserPosts,Link).filter(UserPosts.link_id==Link.id).limit(50).all(): |
There was a problem hiding this comment.
You are not using the u variable anywhere. It would be better if you queried the Songs table (which has entries that definitely have corresponding Spotify IDs). Do a join with the Links table and then get the URLs from there. This would ensure you get 50 songs in a single go.
|
Closing due to inactivity. |
A python script to automate the creation of Spotify playlist and adding songs from Postgres database