-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
added the ability to get all artist's songs as suggested by #366 #389
Conversation
spotdl/spotify_tools.py
Outdated
of the album | ||
:param artist_url - spotify artist ulr | ||
""" | ||
#getting artist uri from url by parsing id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to convert it into Spotify URI. Directly passing artist_url
to spotify.artist_albums
should work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of that. I have made the change.
spotdl/spotify_tools.py
Outdated
text_file = albums[0]['artists'][0]['name']+'.txt' | ||
|
||
for album in albums: | ||
write_album('https://open.spotify.com/album/' + album['id'], text_file=text_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to log.info
the name of the album before writing it to file so the user knows what's going on. Something like log.info('Fetching album: ' + album['name'])
should be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Thanks for this PR @AlfredoSequeida! I left some minor comments. We could merge this once they have been addressed. |
I resolved conflicts introduced in this PR via the merge of #393. |
@ritiek I recall seeing something about functionality for removing duplicate tracks. Has something been done for catching the pull of duplicate albums? I have noticed that sometimes using the function I wrote - there is the possibility of duplicate albums being pulled (even though it results in no duplicate tracks as all the tracks have different IDs). From visiting the track URLs I found that this is because some artists have two albums for the same tracks (one for explicit content and another for non-explicit content). However, this does not show up when browsing Spotify using their GUI client. Here is a good example: https://open.spotify.com/artist/4dpARuHxo51G3z768sgnrY Note that using the Spotify client there are only 3 albums, but when using the Spotify API some albums are downloaded repeatedly. |
From the docs here
I think we could safely default to results from spotify.artist_albums(artist_url, album_type="album", country="US") |
By the way, we should also fetch album types with singles which will require us to make another request with |
@ritiek I will make the new updates for fetching from the US market and including singles. Should I submit them as a new pull request? I am still fairly new to contributing to projects so I do not know what requires a new pull request and what does not. |
…to get all albums from an artist
@ritiek I have made the new updates so when you have the time let me know if I should submit a new pull request. |
Looks great!
You don't need to make another pull request. There is no hard and fast rule for what goes in a pull request but best is to make a separate pull request for each specific feature, bug fix, etc. which correctly implements the original goal of the pull request (fetching all artist albums). Just some minor typos and best if you could auto-format your code with black in this pull request, we could merge this soonish! |
spotdl/spotify_tools.py
Outdated
""" | ||
This funcction returns all the albums from a give artist_url using the US | ||
market | ||
:param artist_url - spotify artist ulr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritiek Fixed
spotdl/spotify_tools.py
Outdated
This function gets all albums from an artist and writes it to a file in the | ||
current working directory called [ARTIST].txt, where [ARTIST] is the artist | ||
of the album | ||
:param artist_url - spotify artist ulr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritiek Fixed
Also, the tests are failing. They should be fixed by calling the correct function ( |
@ritiek Yes that's correct, it has been corrected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ready to merge. Thanks a lot @AlfredoSequeida for the great work!
now all the tracks of an artist can be fetched into a txt file using the -ab option as suggested by #366
spotdl -ab SPOTIFY_ARTIST_URL
a test has also been written & passed