Proposal (WIP): Adding way to get result from all pages for page objects

[chrisvdg]

(Work in progress, already opened a PR to receive feedback)

Adds methodes to get results from page objects without breaking current API

• [x] Implementing page helper methodes
• [x] Implement getting results from all pages in methods that return page objects
• [ ] Add tests
• [x] Add example(s)

closes #75

[chrisvdg]

@zmb3, I shouldn't have breaking changes in this pr, I was fixing up the name of a method I added. But maybe for v2 we could have e.g. GetPlaylistTracksOpts return a slice of PlaylistTrack from all pages by default as to not bloat the client with extra methods like I'm doing now (with adding GetPlaylistTracksAllOpts).

Or are there no plans to do a v2?

[chrisvdg]

@zmb3 , you also want tests for this? Can't immediately see a quick way to test this.

[binford2k]

What's the status of this PR?

[chrisvdg]

Pretty much done, just haven't added any tests for this as I haven't gotten a response from the maintainer. If you like to try it do, please do use my branch and leave feedback of needed ;)

[zmb3]

Sorry for letting this slip. Some thoughts:

• are we sure this needs to be a feature of the library? can we not just have an example that shows how to loop through the pages if that's what you choose to do?
• this PR introduces some inconsistency with respect to how the search results are paged: func (c *Client) NextArtistResults(s *SearchResult) error. We should probably have a uniform way to page through any type of result that comes back from the API.

[binford2k]

As an end user of the library, I'm a little surprised that there's not an auto-paging feature. I'd vote for it to be included.

[zmb3]

@binford2k are you aware of other API wrappers that do provide a similar auto-paging feature? I'd love to look at some for inspiration.

I'm all for building a more uniform and convenient way to navigate from a page to its previous or next page, but I'm not sure a function that gets all pages before returning any data is actually useful.

My concern is that there's not a "get all tracks" endpoint exposed on the API for a reason - pagination is necessary to build performant user experiences, and wrappers like this should map pretty closely to the actual API.

[binford2k]

Sure. I'm not a Go dev, so I don't know what the equivalent would be, but here's a library that we maintain for our own service API. https://github.com/puppetlabs/forge-ruby#paginated-collections

It lazy-loads content with an enumerator, rather than gathering all data at once. Eg, if page size is 20, then accessing index 0-19 is a single network request, but then accessing index 20 requests the next page. There are also .next accessors so you can make very simple while (data = api.next) { render data } type loops.

But I'd honestly be totally OK with a library that provided some form of getAllObjects() that did load everything and just return one giant array, if it were not the default. The author would have to choose that behaviour and the performance implications that come with it.

[doronbehar]

Hey, just a user here who'd like this to get merged somehow, stating my opinion:

The 'lazy load' used in forge-ruby, kind of reminds me of Python's yield. Anyway, my point is that Python and Ruby seem to support this kind of functionality more intuitively. Thinking about a Go equivalent to Python's yield and Ruby's .next, my mind tends to think about channels.

As for examples of Go API wrappers with such a functionality, I'm not familiar with any that support this a dedicated function for that, but for example, Google's Go GitHub API wrapper exemplifies pagination.

[zmb3]

So that Github API is exactly along the lines of what I am thinking. There should be an easy and consistent way to loop through all the pages of a collection, but if the developer wants to do so it is up to them to write the loop. I think we are already there aside from maybe an example and cleaning up some outliers, but I will take a look at this today and see what I can do.

[iMartyn]

This has been open a while... could we see it merged any time soon? I have a preference for an auto-paged setup but if I have to write code, that's fine too, just at the moment, this code is limited to 100 tracks per playlist :(

[zmb3]

@iMartyn I'm just not convinced we need to add any code to the library outside of what's already provided for the index, limit, and offset in the options.

If you have ideas for an easier or more uniform way to support navigating through pages, I'd love to talk about it.

The code in this PR adds a lot of bloat that needs to be repeated for each and every entity that you can pull from the API. It feels like too much.

[zmb3]

I added a more convenient way to move through the pages. You can check out the example here: https://github.com/zmb3/spotify/blob/master/examples/paging/page.go

[jerblack]

This wasn't working for me, but after looking at your implementation I figured out why. If you are using the Fields request option to limit the data being returned in this request to just what's relevant, you have to include the "next" field. Without it, pagination doesn't work and you can only get the first page.

Fields("next,items(added_at,track(id,name,album(id),artists(id)))")