-
Notifications
You must be signed in to change notification settings - Fork 111
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
Load all tracks in "Your Music", "Playlists" and "Top Lists" #279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
=======================================
Coverage 96.64% 96.64%
=======================================
Files 13 13
Lines 1191 1194 +3
=======================================
+ Hits 1151 1154 +3
Misses 40 40
Continue to review full report at Codecov.
|
40c086f
to
3fcbacf
Compare
e8bc47e
to
71d1a03
Compare
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.
It's close to being worth writing a helper to flatten these pages of results. It would be nice if the mocked results could return multiple pages (including maybe some empty ones) to better test the new code. But it does of course all work just fine. I did find it a little slow for my larger "your music" list to load and I would guess that's going to be very noticeable for people on slow machines. We'd need to do some "repagination" to address that but we can solve that another day if it proves to be a problem.
On Mon, Nov 02, 2020 at 10:12:35AM -0800, Nick Steel wrote:
@kingosticks approved this pull request.
It's close to being worth writing a helper to flatten these pages of
results.
agreed. will do.
It would be nice if the mocked results could return multiple pages
(including maybe some empty ones) to better test the new code. But it
does of course all work just fine.
i'll try, but no promises.
I did find it a little slow for my larger "your music" list to load and
I would guess that's going to be very noticeable for people on slow
machines. Some "[repagination]" would be nice but we can solve that
another day.
I think i understand what you mean, but can't comprehend the patch
you've linked.
|
5cd4b2b
to
cc0579c
Compare
@kingosticks: implemented a flatten() helper and modified tests to check
for two pages. i believe this is ready for merge now.
thanks for the review!
|
This is great - thank you. |
fixes #256