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

feat: use the page option to fetch a single page #409

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

jeanettehead
Copy link
Contributor

See the discussion on: #406

Previous to this, the behavior is that specifying a page in options causes an infinite loop of requests and there is no way for calls that are delegated to requestAllPages to be paginated by the api user.

@clayreimann
Copy link
Member

👍 I guess we missed this in our rewrite. Thanks for catching the bug!

@mtscout6
Copy link
Member

@jeanettehead Can you add a test for this so future contributors don't remove this functionality out from under you by accident?

@clayreimann The CI is failing on master again for a change unrelated to this one.

@jeanettehead
Copy link
Contributor Author

@mtscout6 I looked at testing it, but I didn't see any existing tests for the existing pagination behavior, or fixtures with paginated results. Is there something in there that I missed?

@mtscout6
Copy link
Member

I don't think there is existing support for testing the explicit pagination that you are introducing. The other testing there is testing that all data is fetched, regardless of pagination.

@clayreimann
Copy link
Member

@mtscout6 That builds are still failing is related to the flakeyness of github's APIs. To fully solve that problem we need to make sure that all of the tests are fully isolated and then configure the tests to retry a couple times when they fail.

@jeanettehead
Copy link
Contributor Author

@mtscout6 any update on this?

I looked into testing it again, and I'm not really interested in implementing the setup required to test this one line change. The existing tests in user -> list repos are just testing that an array is returned - not that any particular data length comes back, and there no tests at all for the requestable object that was modified. Setting up a new test and fixtures for this seems like a lot relative to the change footprint.

@mtscout6
Copy link
Member

Without tests there will be no guarantee that we don't accidentally remove this in the future. If your ok with that then ok.

@jeanettehead
Copy link
Contributor Author

I'm OK with that.

@mtscout6 mtscout6 merged commit f43d486 into github-tools:master Jan 11, 2017
@mtscout6
Copy link
Member

@clayreimann I don't have permission to push this to npm. Can you do that as a minor version bump?

@clayreimann
Copy link
Member

@mtscout6 I pushed a new minor version. The release script doesn't push to npm directly, we do that from our travis build so you should be able to release versions whenever you like.

@clayreimann
Copy link
Member

e.g. npm run release -- minor

@clayreimann
Copy link
Member

Looks like this broke the build 😢

@clayreimann
Copy link
Member

@jeanettehead

@mtscout6
Copy link
Member

@clayreimann Note that I mentioned earlier that the CI was already broken on master before she submitted this PR: #409 (comment)

@clayreimann
Copy link
Member

Ahh, I guess I misunderstood.

@miyajan miyajan mentioned this pull request Sep 18, 2017
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.

3 participants