search

maxjoehnk / soundcloud-rs

SoundCloud API implementation in Rust
Apache License 2.0
4 stars 3 forks source link

feature: paginated response handler #4

Closed aaronenberg closed 3 years ago

aaronenberg commented 4 years ago

get_collections does the paginated response handler logic and Page models a paginated response. Using generics we can accommodate any type of collection e.g. comments, tracks, playlists.

aaronenberg commented 4 years ago

Found this example of using futures::stream::unfold for pagination. I'll see if I can get something like this working. Let me know if you like this idea.

This allows the user to e.g. get the first 2 pages (client.tracks().stream().take(2).collect().await?) or skip the first page because it already got requested.

Yeah that would be cool if we can cache next_href between .stream() calls.

maxjoehnk commented 4 years ago

Yeah that would be cool if we can cache next_href between .stream() calls.

I think we should skip caching of next_href for now and add it later as it should not impact the public api but introduces additional complexity to this pr.

aaronenberg commented 4 years ago

Hey, I finally had some time in my schedule to work on this. The new logic allows the client to return a Stream of any soundcloud entity. I basically copy/pasted it from these guys.

You've might have seen recently, Soundcloud wrote a blog post announcing they are updating more endpoints to use the new pagination API. When that is complete, the /users/{id}/tracks endpoint, which I've included here, will paginate correctly. Before adding all the other endpoints to the pagination I wanted to get your approval on this approach.

aaronenberg commented 4 years ago

I like how the api turned out. I would prefer if you could also add a way to just get the first page (so instead of calling iter().take(15) just call .get()).

Can we combine these ideas? I think It'd be cool to be able to get the first n pages with .get(..., n).

Also I don't really like the re-export of the Future and Stream Types, maybe you can just use BoxStream from the futures crate?

Thanks. Didn't even know those types existed.

aaronenberg commented 4 years ago

I've added just about every GET resource endpoint (except /me) to the stream api. If there's anything I missed let me know.

maxjoehnk commented 3 years ago

I really like how this API has evolved. This should be ready to merge when we've reduced the redundancy. Thank you for this contribution.

aaronenberg commented 3 years ago

Awesome! Thanks for letting me contribute. This has been an invaluable learning experience for me. If you'd like me to take on any of the changes you've mentioned I'll be happy to.

maxjoehnk commented 3 years ago

I did the changes and added some tests and everything seems to be working fine. I'm still thinking about some api changes but we can merge this for now and open a new PR for further changes.

I would like to reorganize the code and the module structure as some files have grown beyond my preferences. I also think we should rename the iter method to stream and let get return a future instead of a stream. Not sure whether num_pages should be changed to count then.

Do you have anything you want to change regarding this PR? If not I'll go ahead and merge this.

aaronenberg commented 3 years ago

Changes look great. I have nothing more to add to this PR. Cheers!