felix-hilden / tekore

Spotify Web API client for Python 3
https://tekore.rtfd.org
MIT License
207 stars 20 forks source link

Verify current playlist modification endpoints #321

Open felix-hilden opened 9 months ago

felix-hilden commented 9 months ago

We recently removed a playlist method because of a bug, and there was similar discussion on spotipy-dev/spotipy#1075. We should make sure that the current endpoints are feature-complete and up to date.

felix-hilden commented 9 months ago

Also related: spotipy-dev/spotipy#1063

Ch00k commented 8 months ago

I found this issue trying to track down whether tekore has ever supported removing playlist items by index. Apparently it has, according to https://github.com/felix-hilden/tekore/commit/add1f2a48c4d6a0fc1dfb98a8e36f4804be7ebb3 and https://github.com/felix-hilden/tekore/issues/315. As far as I can see, even though the functionality is not documented here, it still works: you can delete tracks with indexes providing a body like this:

{
  "tracks": [
    {
      "uri": "spotify:track:6YowAxI2HIbhAwumOqpldE",
      "positions": [7]
    }
  ]
}

I verified this at https://spotify-dedup.com/ (it sends Spotify API requests right from the browser, so it's easy to verify).

I am currently working on an app that, among other things, allow removing duplicate tracks from playlists, and without being able to remove playlist items by their index the workflow is much more involved than I'd like it to be: I have to remove the track by ID, which removes all its occurrences, then re-add the same track to the position of its first occurrence. And that's not to mention a full backup of the entire playlist in question, in case something goes wrong along the way.

Is there a story behind why #315 came to be? My guess is that it probably was https://github.com/spotipy-dev/spotipy/issues/1063. Is there a reason tekore would not want to bring back the playlist_remove_occurrences method, removed in https://github.com/felix-hilden/tekore/commit/add1f2a48c4d6a0fc1dfb98a8e36f4804be7ebb3, even though the official API docs don't mention the functionality?

felix-hilden commented 8 months ago

We actually got errors in our tests from Spotify. The logs will expire soon, but here it is (400: Could not remove tracks, please check parameters.). The previous body only included the positions parameter, but that's interesting if it works! Thank you 👌 Would you be willing to contribute a fix?

Ch00k commented 8 months ago

Hm, interesting. The failed test was calling playlist_remove_indices. playlist_remove_occurrences call however, which in the test is made before playlist_remove_indices, succeeded.

I'll create a PR to add the playlist_remove_occurrences back. I'll also verify if playlist_remove_indices works, and add it back too if it does.

Since this functionality is undocumented, I think it would be convenient for the user to know about this. I was thinking about something along the lines of a RuntimeWarning if the user calls a function that utilizes undocumented features. What do you think?

felix-hilden commented 8 months ago

Oh, fair! I may have jumped the gun with that when I saw it wasn't documented anymore. And that sounds good. RuntimeWarnings are a bit much. I've resorted to just documenting the fact that it's not mentioned officially. So a note in the docstring would be enough for me! Thank you!

felix-hilden commented 7 months ago

remove_indices still not working, but the old remove_occurrences does! I couldn't find any other things we've missed though.