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

Support backwards pagination with list's #auto_paging_each #865

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Oct 9, 2019

Previously, #auto_paging_each would always try to paginate forward,
even if it was clear based on the list's current filters that the user
had been intending to iterate backwards by specifying an ending_before
filter exclusively.

Here we implement backwards iteration by detecting this condition,
reversing the current list data, and making new requests for the
previous page (instead of the next one) as needed, which allows the user
to handle elements in reverse logical order.

Reversing the current page's list is intended as a minor user feature,
but may possibly be contentious. For background, when backwards
iterating in the API, results are still returned in "normal" order. So
if I specifying ending_before=7, the next page would look like [4, 5, 6] instead of [6, 5, 4]. In #auto_paging_each I reverse it to [6, 5, 4] so it feels to the user like they're handling elements in the
order they're iterating, which I think is okay. The reason it might be
contentious though is that it could be a tad confusing to someone who
already understands the normal ending_before ordering in the API.

Fixes #864.

r? @ob-stripe

cc @ruanltbg Can you also take a look at this since it sounds like
you've got a good handle on how API list ordering works? Thanks!

cc @stripe/api-libraries

Previously, `#auto_paging_each` would always try to paginate forward,
even if it was clear based on the list's current filters that the user
had been intending to iterate backwards by specifying an `ending_before`
filter exclusively.

Here we implement backwards iteration by detecting this condition,
reversing the current list data, and making new requests for the
previous page (instead of the next one) as needed, which allows the user
to handle elements in reverse logical order.

Reversing the current page's list is intended as a minor user feature,
but may possibly be contentious. For background, when backwards
iterating in the API, results are still returned in "normal" order. So
if I specifying `ending_before=7`, the next page would look like `[4, 5,
6`] instead of `[6, 5, 4]`. In `#auto_paging_each` I reverse it to `[6,
5, 4]` so it feels to the user like they're handling elements in the
order they're iterating, which I think is okay. The reason it might be
contentious though is that it could be a tad confusing to someone who
already understands the normal `ending_before` ordering in the API.

Fixes #864.
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very clean implementation and nice tests 👍

@ruanltbg
Copy link

I can also confirm that now it is working properly. Thank you @brandur / @brandur-stripe. For fast implementation.

@brandur-stripe
Copy link
Contributor

Thanks both!

@brandur-stripe brandur-stripe merged commit e3cc91d into master Oct 10, 2019
@brandur-stripe brandur-stripe deleted the brandur-backwards-auto-paging-each branch October 10, 2019 17:11
@brandur-stripe
Copy link
Contributor

Released as 5.7.0.

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

Successfully merging this pull request may close these issues.

auto_paging_each should paginate back when ending_before is used
5 participants