Add support to search multiple markets at once. #526
Conversation
|
Will adjust formatting to try and resolve those checks in just a bit - local flake8 didn’t find anything |
spotipy/client.py
Outdated
| results[type + 's']['items'] += result[type + 's']['items'] | ||
| results[type + 's']['total'] += result[type + 's']['total'] | ||
| if total and len(results[type + 's']['items']) >= total: | ||
| # sice 'items' to only include number of results requested |
There was a problem hiding this comment.
Yes. Will fix that one!
|
Do you have any improvement idea on this maybe @ritiek? Otherwise really happy to merge |
| results[type + 's']['href'].append(result[type + 's']['href']) | ||
| results[type + 's']['items'] += result[type + 's']['items'] | ||
| results[type + 's']['total'] += result[type + 's']['total'] | ||
| if total and len(results[type + 's']['items']) >= total: | ||
| # splice 'items' to only include number of results requested | ||
| results[type + 's']['items'] = results[type + 's']['items'][:total] | ||
| return results |
There was a problem hiding this comment.
It seems weird to me here that we're messing with the API response returned by Spotify. This could be confusing at times.
I think a better choice would be to have a key for each market in the result. This way we do not mess with the internal response returned by Spotify. The results would then look something like below on using sp.search:
>>> result = sp.search("lany good guys", market=["US", "GB"])
>>> us_tracks = result["US"]["tracks"]
>>> us_tracks.keys()
dict_keys(['href', 'items', 'limit', 'next', 'offset', 'previous', 'total'])
>>> us_tracks["href"]
'https://api.spotify.com/v1/search?query=lany+good+guys&type=track&market=US&offset=0&limit=10'
>>> gb_tracks = result["GB"]["tracks"]
>>> gb_tracks["href"]
'https://api.spotify.com/v1/search?query=lany+good+guys&type=track&market=GB&offset=0&limit=10'@stephanebruckert what do you think?
There was a problem hiding this comment.
I completely agree - It feels hacky. I was actually thinking of implementing the market key idea you mention as I was working on it, but I wanted to try and preserve the format of the response from Spotify.
There was a problem hiding this comment.
Sorry I merged this too quickly. I agree, it does feel weird. Now that you point it out it's true that mixing up results does not make sense, mainly due to the fact that results are supposed to be ordered by "relevancy".
The reason I thought it was OK in the first place is that in many cases, we are only interested in the first result. In this specific case, getting a dictionary of all countries can make it difficult to find in which country the release was found.
But it's probably nicer to let developers handle country results the way they want, since we don't really know what their use case is. So I would be all for this change
There was a problem hiding this comment.
@stephanebruckert It's all good. Thank you for maintaining spotipy all this while! I agree that devs should be able to do whatever they want with the country results later on.
@nleroy917 Are you willing to make a new PR for this?
There was a problem hiding this comment.
Yes, definitely - I can work on that
Resolve issue #525
Overview
Detailed
I reformatted the API interface
searchmethod. I did notice that all but one method directly returned the result that comes from theself._getcall, and I wanted to keep this to retain any abstraction, but I couldn't think of a way around it. The newsearchmethod on the Spotify client keeps all original functionality, but I added in some logic to handle if a user passes in a list of markets to search through. In addition, as referenced in #525, I added in the ability to search ALL markets as well as another available parameternthat will stop the searching once a given number of results are found.I really tried to keep the object structure returned from Spotify... So, the final object returned from
searchwhen multiple markets are passed in will be almost identical to the object returned if one market or no markets are passed in.To the best of my knowledge, it is the same, except
hrefnow results in a list ofhrefs used for searching.Performance Issues
As mentioned by @stephanebruckert, this code performs poorly due to looping through http requests, but I can not think of a way around it.