Skip to content
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

naming of ListResponse filters "before" and "after" is counterintuitive #34

Closed
oschonrock opened this issue Oct 19, 2017 · 2 comments
Closed

Comments

@oschonrock
Copy link

oschonrock commented Oct 19, 2017

related to #32

Our commercial use case is:
retrieve all events since the last EVENT_ID we have processed (pretty common?)
ie chonologically speaking we want "all events AFTER" that EVENT_ID

However, to get that set of records we need to pass
["params" => ["before" => "SOMEEVENTID"]] to ->list() or ->all()

Note "BEFORE" filter must be used.

The reason seems to be that the API returns events in REVERSE chronological order and therefore all the filter query params are inverted from the natural meaning in "time". AFTER = BEFORE and BEFORE = AFTER

Its gets worse when when trying to de-paginate results and using ListResponse->before and ListResponse->after which are also (logically) inverted.

Questions/Suggestions:

  • Why return events in REVERSE chronological order in the first place? We would never want to process them in reverse! (that obviously wouldn't work commercially)

  • Suggest to at least provide a backward compatible "sort_order" => "ASC" param which gives events (or any other paginated result) in FORWARD chronological order

Then ensure that "before" and "after" do what they say, ie

list('after' => SOMEID) gives first "limit" records chronologically after that ID, and if we then call
list('after' => previousListResponse->after) it gives us the chronologically next "limit" events

or even better, make Paginator do this for us automatically ref #32

@oschonrock oschonrock changed the title naming of paginated lists filters "before" and "after" is counterintuitive naming of ListResponse filters "before" and "after" is counterintuitive Oct 19, 2017
@timrogers
Copy link

@oschonrock Thanks for your feedback - we really appreciate it!

I don't think it's necessarily clear that either standard or reverse is universally better - it just depends on your use case (for example reverse chronological will usually make sense to show in a dashboard-type UI) and what exactly you're doing in your API integration. But I can definitely understand why it's confusing at the moment for what you're trying to do!

I think the key here is for us to make it clearer what "before" and "after" mean - that we're talking in terms of the notional stack of pages (which has the most recent things at the top) rather than in strict chronological terms. We should also be really clear that our pagination is reverse chronological.

You could consider building a small wrapper yourself around the library's listing functionality if you wanted to get a consistent, easy to use experience on your side.

@paprikati
Copy link
Contributor

I'm going to close this issue, as we are unlikely to change this behaviour. It's consistent across our API, and hopefully our docs are clear about how these filters work. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants