- Notifications
You must be signed in to change notification settings - Fork786
feat: use the page option to fetch a single page#409
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Byron Anderson <banderson@pivotal.io>
clayreimann commentedDec 21, 2016
👍 I guess we missed this in our rewrite. Thanks for catching the bug! |
mtscout6 commentedDec 21, 2016
@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 commentedDec 21, 2016
@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 commentedDec 21, 2016
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 commentedDec 21, 2016
@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 commentedJan 9, 2017
@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 commentedJan 11, 2017
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 commentedJan 11, 2017
I'm OK with that. |
mtscout6 commentedJan 11, 2017
@clayreimann I don't have permission to push this to npm. Can you do that as a minor version bump? |
clayreimann commentedJan 11, 2017
@mtscout6 I pushed a new minor version. The |
clayreimann commentedJan 11, 2017
e.g. |
clayreimann commentedJan 11, 2017
Looks like this broke the build 😢 |
clayreimann commentedJan 11, 2017
mtscout6 commentedJan 11, 2017
@clayreimann Note that I mentioned earlier that the CI was already broken on master before she submitted this PR:#409 (comment) |
clayreimann commentedJan 11, 2017
Ahh, I guess I misunderstood. |
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.