Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for playlist management (v2) #287
base: main
Are you sure you want to change the base?
Support for playlist management (v2) #287
Changes from 27 commits
95c1031
5333536
c23eda1
0acd636
d449225
1dc0e7e
13e98bf
2994325
42a6506
ba0a616
46f2935
e9caa71
4ec107c
74d405f
d6708ee
525f420
e8ce572
4adc6f4
4b3a3b4
468a811
8af448b
b3c09d1
6fd0a7b
3ec1c44
6237b5c
79317af
609795f
47c553f
f8c69f7
fc11efe
0d596ed
400060e
50f1ab3
1c0358a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this belongs in
web
. I think of it as: "if we ripped outweb
into a stand-alone module, would this be useful to include?".In fact, I think it'd also want the second half of
save()
(and then get rid of_replace_playlist()
since it's serving little purpose).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.
Arguably worth pulling out first half of
playlist_lookup
into a separate function which always returns a tuple of(Playlist, owner)
. We could then avoid doing all the pointless libspotify stuff in our case here. And avoid this optionalextra_owner
param.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.
Why does it only get used in the replace strategy case? And can't we do this for both playlists?
I agree a backup mechanism is nice and I appreciate this, combined with the M3U backend, provides a simple mechanism to take another shot at saving the new playlist. But this is a little complicated considering it's reimplementing parts of Mopidy-M3U (why do we need length and artists? Should use
Path
. Why binary mode? Should be ".m3u8") and still needs a manual step to move the backup so Mopidy-M3U can find it. Could we consider something simpler? Maybe just dump (pickle?) bothsaved_playlist
andplaylist
to disk? And then potentially adding a Spotify playlist restorationCommand
later?Whatever is decided, it should be pulled out into its own function.
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.
because truncation can only happen when we use the replace strategy. i don't think its important to make a backup when patching the playlist. in the latter case, the user will see an error that their change failed, and the playlist is unmodified. but when using replace (and the very specific situation in the code comment happens; see last paragraph), the old playlist is clobbered, and we back it up in that case.
not sure what 'both'/other playlist you mean. the new state? this feature was only intended to avoid permanent data loss of previously existing data, hence i didn't implement saving the new state. (but see last big paragraph for more info)
afaik, m3u-ext requires the length, and filling it in doesn't cost us anything. artist (and the whole extm3u block itself) were added to make the file human-parsable to an extent in this emergency situation.
i think i am with
Extension.get_data_dir()
?i believe a remnant of a previous iteration. will change.
i believe he likeliness of this code ever being called is extremely low. for it to trigger*, we first have to make a successful PUT request, and then fail at a later POST request to the same endpoint, with the same authentication scope and the same parameters. and in normal use, replace mode will not be used at all, since no client supports batching multiple edit operations anyways (for a single edit, patch mode will always be used, since it's safer and requires the same amount of api requests). this is not something the user should have to deal with on a regular basis, so 'comfort features' are not included. consider this feature due diligence.
* i'm actually creating the backup file not just when we truncate the playlist, but everytime something goes wrong with replace mode, since number of requests made is not available at this level. given that the code is pretty much never going to get called anyways, i didn't bother tightening down the check.
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.
OK, truncation is one class of issue but isn't data loss possible in either strategy? If a replacement is required and the remove operation succeeds but the corresponding add operation fails, won't that still leave you with a half modified playlist which is neither what we had or what we wanted? A backup of
playlist
in both cases would at least always leave you with a copy of the requested new state somewhere. Am I missing something?Your code here makes a backup of
playlist
(the requested new state), notsaved_playlist
(the old state). A request failure somewhere can leave us in a mess with our previous existing data now lost. So I was suggesting saving both playlists to give the best choice when it comes to recovery.No, there's no spec (yay!) and we (Mopidy-M3U) do not require it or even use it. A list of URIs is all that is required to be useful.
Sorry, I meant
pathlib.Path
like the implementation in Mopidy-M3U.I think we've covered this before, few clients have any sort of playlist editing. We should support what is possible with the Mopidy API, not how we think people should/might/currently use it. Someone also uses it in a way you didn't think they would/should.
Or we have a quota/rate limit issue half way through, or something else unlikely. But it if can happen it will happen.
Maybe we should remove this code from the PR and address it later if anyone in the world ever sees (and reports) a problem.
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.
technically yes, but in my opinion only truncation is severe enough to care about. i have no problem with dropping the strategy test; will implement.
will implement.
useful to a computer; but i wanted it to be as useful as possible to humans as well. if we were to end up in this state, let's be as forthcoming to the user as possible.
and Extension.get_data_dir is returning pathlib.path. we must really be talking about different things..?
strongly disagree. we should have some defence against truncation, since it's irreversible data loss. the trade-off is a few lines of imperfect code vs possible thousands of curated songs. if i were to trust mopidy-spotify with editing my years-old playlists, and it deleted most of their contents, and the maintainers just said "sorry, but thanks for the bugreport", I'd be majorly pissed. it can still be improved after committing.