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

Fix unnecessary request in sync pagination #483

Merged
merged 1 commit into from
May 25, 2024

Conversation

thirteenowls
Copy link
Contributor

Description

Makes use of the next field to end pagination early, instead of waiting for Spotify's API to return no data.

Motivation and Context

Currently, PageIterator always sends one extra request to the API, because it does not check next to know if more data is available (unlike its async equivalent).

Dependencies

None.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

This has been tested by adding a dbg! call right before sending a request in PageIterator's impl, then running the pagination_sync example with and without this change.
Tests were run using an account with 51 saved tracks, which should require two requests with limit set to 50.

This is pagination_sync's output before the change:

Items:
[src\clients\pagination\iter.rs:56:9] self.offset = 0
* Track 1
-snip-
* Track 50
[src\clients\pagination\iter.rs:56:9] self.offset = 50
* Track 51
[src\clients\pagination\iter.rs:56:9] self.offset = 51

And this is its output after the change:

Items:
[src\clients\pagination\iter.rs:56:9] self.offset = 0
* Track 1
-snip-
* Track 50
[src\clients\pagination\iter.rs:56:9] self.offset = 50
* Track 51

The output is the same, but the extraneous request has been eliminated.

Is this change properly documented?

N/A

@ramsayleung
Copy link
Owner

Thanks for your contribution, the CI tasks are failed, would you consider to fix them?

@thirteenowls thirteenowls mentioned this pull request May 22, 2024
4 tasks
@thirteenowls
Copy link
Contributor Author

Sure. The failure is unrelated to this contribution, so I've opened a new PR. I'll rebase this one on it once it's merged.

@ramsayleung ramsayleung merged commit ab2c606 into ramsayleung:master May 25, 2024
7 checks passed
@ramsayleung
Copy link
Owner

Merged, thanks for your contribution.

@thirteenowls thirteenowls deleted the fix-extra-request branch May 25, 2024 05:40
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

Successfully merging this pull request may close these issues.

2 participants