Skip to content
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

updated functionality of playlist_add_items() #914

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

oliveraw
Copy link
Contributor

@oliveraw oliveraw commented Dec 5, 2022

playlist_add_items() should not be able to accept pure IDs, since the type 'track' or 'episode' cannot be inferred. We are proposing a change where playlist_add_items is modified to only accept URIs and URLs

… urls (ids should not be allowed since 'track' or 'episode' cannot be inferred purely from the id)
@oliveraw
Copy link
Contributor Author

oliveraw commented Dec 5, 2022

The issue was first mentioned in #810, wherein episodes could not be added to playlists by ID or URL (only by URI). The proposed solution allows for both track and episode URIs and URLs, but no IDs. We have also added another (deprecated) function, user_playlist_add_episodes(), in order to match the existing user_playlist_add_tracks().

@stephanebruckert
Copy link
Member

Thanks for that!

That's a fairly annoying bug as the fix would be backward-incompatible. If merged now it will likely break things for a majority of apps dealing with plain track ids and fix things for only a minority of apps dealing with plain episode ids. That's probably a good reason to keep it for the next major version (v3) meaning it can't be merged in the current master branch (v2).

To get the best of both world my suggestion would be to open 2 PRs:

  • one against the current master branch that we will then release as v2.23.0 :
    • leave playlist_add_items's functionality as it is but update the docstring to let people know that episodes IDs are expected to be full URIs
    • still add the new deprecated user_playlist_add_episodes so that people can use the feature without full URIs
  • one against the v3 branch that we will release as v3.0.0:
    • fix playlist_add_items to handle both as done in the current PR
    • no need for the deprecated ones anymore

The first PR would be optional because only informative, but if doing both my recommendation is to start with the first one, and then base the v3 update from it.

@stephanebruckert
Copy link
Member

Thanks for the first PR in #919

I will try to rebase the v3 branch against master ASAP, this way it will be a bit easier to do the other changes

@oliveraw
Copy link
Contributor Author

sounds good, will keep an eye out!

@stephanebruckert
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants