-
Notifications
You must be signed in to change notification settings - Fork 20
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
Bring back removing playlist item occurrences API #322
Conversation
str | ||
snapshot ID for the playlist | ||
""" | ||
gathered = {} |
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.
IMO this method could be simpler:
def playlist_remove_occurrences(
self, playlist_id: str, refs: dict[str, list[int]], snapshot_id: Optional[str] = None
) -> str:
tracks = [{"uri": uri, "positions": indices} for uri, indices in refs.items()]
return self._generic_playlist_remove(
playlist_id, {"tracks": tracks}, snapshot_id
)
This of course changes the signature, but maybe that is not an issue, given that it probably has never been used by anyone?
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.
Yep! It's really just a choice of how to design the API. And arguably if you want to remove multiple occurrences of the same track that makes more sense (and it's closer to the true API which is nice). Let me think about it a bit.
In any case, if we change it, maybe coming up with a new name would be nice just for people who are upgrading from an old version. Just good documentation would also help to note that this has changed. Let's see.
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 think I might have a clue as to why refs
was made a list
instead of a dict
: it could be because of chunking. I have a use case where I need to remove large number of tracks from a playlist, and that is currently failing with HTTP error 413, because this method does not support chunking (it does not have a @chunked
decorator). I tried adding the @chunked
decorator to the method, but that does not seem to work with ref
being a dictionary.
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.
So I tested this with chunking, and the way it works is not like the rest of the endpoints. If you want to use chunking with this one, you must provide a snapshot_id
, so all chunks will be executed against the same version of a playlist. It does make sense when I think of it: if you've removed a bunch of positions in the first chunk, the next chunk's positions would not match anymore.
I am not sure how to implement this. We could do something like this I suppose
if len(refs) > chunk_size and snapshot_id is None:
raise RuntimeError("For chunked requests snapshot_id must not be None")
The tricky part is that we do not have access to chunk_size
in the method itself, so this logic needs to go into the chunked
decorator implementation. What do you think?
.. warning:: | ||
This feature is undocumented and may stop working at any time. |
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.
For valid rST:
.. warning:: | |
This feature is undocumented and may stop working at any time. | |
.. warning:: | |
This feature is undocumented and may stop working at any time. |
str | ||
snapshot ID for the playlist | ||
""" | ||
gathered = {} |
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.
Yep! It's really just a choice of how to design the API. And arguably if you want to remove multiple occurrences of the same track that makes more sense (and it's closer to the true API which is nice). Let me think about it a bit.
In any case, if we change it, maybe coming up with a new name would be nice just for people who are upgrading from an old version. Just good documentation would also help to note that this has changed. Let's see.
|
||
@scopes([scope.playlist_modify_public], [scope.playlist_modify_private]) | ||
@send_and_process(top_item("snapshot_id")) | ||
def playlist_remove_indices( |
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 tried running the tests locally, and this still fails for me. Did you find an alternative?
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 has worked for me when tried to use this method in my app. I have not tried the test though, but I will.
I also just fixed some tests on master if you want to pull them and run them on your own! |
78da661
to
894c0b4
Compare
894c0b4
to
a674afd
Compare
SpotifyPlaylistItems.playlist_remove_indices
andSpotifyPlaylistItems.playlist_remove_occurrences
were removed as part of #315 due to the corresponding functionality being removed from Spotify Web API documentation, however, the functionality still works.This change adds those two methods back, marking them as undocumented in tekore's own documentation. This (partially?) addresses #321,
Related issue: #321
tox
checks passed with an appropriately configuredenvironment