Support for playlist management (v2) #287
Conversation
It also addresses the comments in mopidy#236
- can't pass in sets, only lists - provide a useful default n parameter
making playlist private, not just for the obvious reason, but also because our API client is not authorized for the playlist-modify-public scope (only playlist-modify-private).
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
==========================================
+ Coverage 96.68% 97.10% +0.42%
==========================================
Files 13 13
Lines 1207 1418 +211
==========================================
+ Hits 1167 1377 +210
- Misses 40 41 +1
Continue to review full report at Codecov.
|
mopidy_spotify/translator.py
Outdated
| # Note: date can by YYYY-MM-DD, YYYY-MM or YYYY. | ||
| date = web_album.get('release_date', '').split('-')[0] or None | ||
|
|
||
| return models.Album(uri=ref.uri, name=ref.name, artists=artists, date=date) |
There was a problem hiding this comment.
I had to extend the track translator to parse release dates (just the bare minimum (year); spotify also supports finer dates) to support round-tripping between the web and native formats. otherwise, I wouldn't have been able to compare output from save() and lookup().
girst
force-pushed
the
bug22
branch
2 times, most recently
from
0f18224
to
6c5d9a0
Compare
|
I found and fixed the problem with the cache (the playlist's tracks are fetched from two endpoints, not just the one I cleared). While at it, I also moved the newly added code to use the new shorter playlist URIs that spotify offers (as the old ones are deprecated). Also started testing this out on my personal spotify account, and it pretty much works. This leaves us with only one major item to figure out: what do do on errors. For this, I'd like to solicit some ideas from you guys. |
not thoroughly tested yet
Uses Myers' diff, extended to detect moved ranges. With the exception of non-consecutive deletes, this generates a near-optimal edit script.
To use the preexisting mopidy_album_mock the web translator had to be extended to parse release dates (only the year is supported, as that's what the test data requires).
op.__repr__ is useful for debugging only.
this won't ever fire, since getattr() will raise on error.
Rest will be fixed by black.
|
OK, so I think this is done. I'm going to add @jodal, @adamcik and @kingosticks as potential reviewers. Looking forward to your feedback! PS: in case you feel initmidated by the patch size: 70% of it is just tests. edit nov21: just for fun, i did some calculations (on commit 468a811): only 230 lines or 19% of this patch is actual non-test code (excluding comments, closing braces and blank lines): |
girst
force-pushed
the
bug22
branch
2 times, most recently
from
52e64c4
to
c34a7b0
Compare
I agree, responses at this level would be unpleasant and also not useful. But testing multiple things isn't ideal. I think we can restructure a little and test at lower levels to avoid writing too many Responses and hopefully also avoid a Python version of the Web API. It may end up being more boilerplate but I'll have a go and see where I land. Either way, doing this will probably help me review. |
|
Sorry, I did a few bits, hit a small issue, managed to context switch along the way and lost my momentum. Would you mind rebasing on master when you get a spare moment. |
|
On Sun, Jan 10, 2021 at 09:25:49AM -0800, Nick Steel wrote:
Sorry, I did a few bits, hit a small issue, managed to context switch
along the way and lost my momentum. Would you mind rebasing on master
when you get a spare moment.
thanks for those "few bits!" if there's anything i can do to get you
back up-and-running, shout now ;)
pulled your changes in (and propagated them to the new endpoints).
|
|
I've got another review in progress but it's not quite done yet. Probably less work for you if it's all at once. |
|
Hey guys, what's the current status with this PR? |
|
It's not yet merged. It's waiting on me, it's my fault. I've been meaning to dig this out for weeks and I have only a couple of days before I'm unavailable for a while. Nothing motivates like an impending deadline. |
|
I'm still interested to get this merged; it's just that since getting it
to work for myself, the pressure is off. also, work has been a bit more
time and energy consuming, in the last couple of months (although i
expect that to be over relatively soon).
On Mon, May 03, 2021 at 02:57:04PM -0700, Nick Steel wrote:
It's not yet merged. It's waiting on me, it's my fault.
don't beat yourself up over it. we're all doing this as a hobby! if you
don't have the time, or just plain don't feel like working on it, there
are no hard feelings from me :)
|
|
We've all spent a lot of time on this and it deserves to be finished; I think in your shoes I might have been quite annoyed so I mainly want to say I really appreciate the patience and understanding here. I did lose momentum and focus on this, and have also scaled back how much time I invest in this hobby but this kind response is exactly the motivation I need. Having said that, I'm about to take on an enormous priority shift (complete with a couple of weeks off work and zero free time) so while I would love to say I'll properly get back into this right now but I'm not sure that's remotely feasible for a few weeks. |
|
@kingosticks any chance we could get this approved? |
|
I spent time reviewing this and reworking the tests to fit the way we had done things previously - I don't want to maintain the code as it is. I have that local branch somewhere, I have been meaning to pick it up for the last 10 months I just have not had the time to invest and get back into it. So yes, there is a chance to merge this in a slightly different form. I should publish that branch and then someone can finish it off rather than wait any longer. |
|
On Tue, Mar 22, 2022 at 10:00:44AM -0700, Nick Steel wrote:
I spent time reviewing this and reworking the tests to fit the way we
had done things previously - I don't want to maintain the code as it
is.
if you give me some direction on what you want done differently, i'd be
happy to work on it again :)
|
This is now done :D
I've adapted Myers' diffing algorithm (the one that's used by e.g. git-diff) to support moving ranges of tracks. It generates a relatively optimal edit script, to minimize API requests without the diffing algorithm taking too long. Especially getting moves to work was quite tricky (took me over a week of daily head-scratching), but I have written a whole bunch of tests for it. the web_client_mock now also supports editing playlists.
One thing we haven't yet talked about is what to do when API requests fail. The worst case (and only serious one) is when we clear-and-repopulate a playlist. When we fail mid-way through, we lose data. Should we do something (e.g. save an m3u playlist somewhere) then?
What's still todo is:
CC: @blacklight
closes #22, closes #236