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

Stop paginating after finding a matching parameter. #122

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Sep 3, 2018

No description provided.

@jmcarp
Copy link
Contributor Author

jmcarp commented Sep 6, 2018

Ping @nickatsegment 👋

@systemizer
Copy link
Contributor

Nice catch! Looks like a bug to me.

However, we don't want to return true if we don't find the parameter. I think we want to keep ~lastpage so that we keep iterating in case we don't find the parameter on the first page

@jmcarp
Copy link
Contributor Author

jmcarp commented Sep 6, 2018

Looking at the sdk docs, pagination continues until either the callback returns false or there aren't any more pages to fetch. So I think this patch is correct--we bail out (return false) if we find the matching parameter, else continue paging if there are more pages (return true). We could continue returning !lastPage, but that wouldn't actually change the behavior of the function. IMO this way is clearer (the lastPage parameter isn't really useful to us), but happy to revert that part if you prefer.

Does that make sense? Sorry if I'm misunderstanding.

@systemizer
Copy link
Contributor

Oh I misread. That works, too.

The way I saw it is: you are fixing the case to break out of the pagination when we find the parameter. Otherwise the behavior of returning !lastpage and true is essentially the same thing (keep iterating over all pages until a break case).

This looks good to me. thanks for the contribution!

@systemizer systemizer merged commit 29a1f2b into segmentio:master Sep 7, 2018
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