Add Async and Paginated Functions

[probably-not]

This solves #248 by exposing pagination functions and async functions (via callbacks).

[gnvk]

@probably-not First of all, I'm super happy for this PR and that you reported #248. Sorry for the late response.

I would like to address your issue in a way that it's most useful for all of our users.

Here are my thoughts:

• I cannot come up with a scenario where handling pagination manually is beneficial (over the async method). Can you? Because if we can't, I really think we shouldn't complicate the API with the "...Paginated" functions.
• The interface of the Async functions you introduced is a bit confusing in my opinion.
  • TotalLimit is not really a total limit, but the limit of the list passed to the callback.
  • The errors coming back from the (per page) requests are passed to callback, but really, you'll never be able to continue after an error, because you won't know the next page token. There are two problems with this: 1. you have to unnecessarily handle errors twice and 2. you won't be able to continue from where you left of after an error.
  • There are no "Multi" versions.

This is what I recommend to address these issues:

• Simply add PageToken to all requests.
• Add the following async functions:
  • GetTradesAsync(symbol string, req GetTradesRequest, callback func(trades []Trade, nextPageToken string) (keepGoing bool)) error
  • GetMultiTradesAsync(symbols []string, req GetTradesRequest, callback func(trades map[string][]Trade, nextPageToken string) (keepGoing bool)) error
• The TotalLimit / PageLimit request parameters should behave the same way for these async functions.

What do you think?

[probably-not]

Hey @gnvk, sorry for the slight delay in answering here.

Re your thoughts:

I cannot come up with a scenario where handling pagination manually is beneficial (over the async method). Can you? Because if we can't, I really think we shouldn't complicate the API with the "...Paginated" functions.

Honestly, I'm not sure if I can come up with a reason for having the paginated functions separate, other than just for manually implementing pagination. I mostly added it so that there could be parity with the actual available API (if the API allows pagination then I generally want there to be pagination available in the SDK), and to not touch the already available functions (I didn't want to change signatures on the already available functions to return the next page token).

That being said, if you don't think there's a use case for returning the next page token to the user, then the functions can be removed and we can allow the pagination strictly through the Async functions.

The interface of the Async functions you introduced is a bit confusing in my opinion.

Yeah, that makes sense. I just threw together what I needed specifically for my use case, basically wanted input from you guys for what the actual signatures would be and how you wanted it to be in the SDK.

Re implementation recommendations:

Simply add PageToken to all requests.

This is fine for letting the user add a page token to the request, but the issue is more that the response (for the normal API calls) doesn't contain the next page token... so adding the PageToken to all of the normal API call requests is kind of a moot point... If there's no way for the user to get the next page token, how are they to know what page token to add to the request?

This is more of a personal preference, but I don't really love SDK types that don't allow the full functionality of all of their fields... If I see a type with a PageToken on it, I expect there to be a way for me to get the NextPageToken so that I can paginate. (this is a personal preference of course... so the question is really more for you guys in how you want the SDK implemented).

Add the following async functions

I can adjust the Async functions to match this call signature. The point I mentioned above is mostly my concern for using the same type...

In addition to that, we aren't returning a page token on the function, just on the callback, so there's no easy way for setting the next page token on the request (without holding a separate variable and setting that variable outside of the callback... which can be a little tricky and occasionally racy for beginners in Go). I think maybe returning the next page token on the function itself (when keepGoing is false) would be a simpler way, so that anyone using the function would get the page token back in the response, and there wouldn't need to be any tricky/racy flow in setting variables inside the callback.

Let me know what you think, obviously this is your SDK at the end of the day, so whatever you decide in terms of signatures/types, I'm happy to make the adjustments!

[probably-not]

@gnvk Any thoughts on my response above?

I've been keeping my branch up to date but since there are ongoing changes I don't want to have continuous conflicts...

[gnvk]

@probably-not I'm sorry for being so hesitant about this PR. After discussing it with the team internally as well, we concluded that we would not like to expose the page token on the Go API (neither in the request, nor in the response). As you wrote:

That being said, if you don't think there's a use case for returning the next page token to the user, then the functions can be removed and we can allow the pagination strictly through the Async functions.

We really can't think of a real-world use case for the paginated functions, and we would not like to maintain 2 more functions per type for no reason.

Moreover, the word Async is a bit misleading for your current implementation, because those functions are actually blocking (unlike the old "async" functions in v2). This is fine though, only the wording should be changed, for example GetTradesPaged or GetTradesPerPage or GetTradesWithCallback.

So the interface should be something like:

GetTradesWithCallback(symbol string, req GetTradesRequest, callback func([]Trade) bool) error
GetMultiTradesWithCallback(symbols []string, req GetTradesRequest, callback func(map[string][]Trade) bool) error
// same pairs for all other types

Please let me know if you're interested to update this PR based on this. If yes, that's great 😃 If not, that's also totally fine, I'm happy to do it.

[JayBusch]

I don't understand why this issue was closed. It appears to me that the SDK only allows internal paging - i.e. a large request for record is returned to a single slice this requiring for the entire data set to be both returned and held in memory.

@probably-not seems to have provided a good solution. If that solution is not perfect why not adapt the changes and merge with this repo? Not supporting a basic function like manual paging appears to be a serious deficiency of this SDK.

Am I missing something about how to use this SDK? If I request a very large number of historical trades, will I not have a very large amount of memory used? It seems that being able to manually page and free the memory after doing something like writing the data to DB would make sense. I would be happy to submit a PR myself but it seems you do not want to support manual pagination at all. Please correct me if I am wrong.

[gnvk]

@JayBusch I answered your questions under #248