GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: use the page option to fetch a single page by jeanettehead · Pull Request #409 · github-tools/github

Conversation

@jeanettehead

@jeanettehead jeanettehead commented Dec 20, 2016

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Jeanette Head
feat: use the page option to fetch a single page
21fe6b6
Signed-off-by: Byron Anderson <[email protected]>
@jeanettehead jeanettehead mentioned this pull request Dec 20, 2016
[Feature Request] Ability to fetch a single page of items in a list #406
Open
@clayreimann

clayreimann commented Dec 21, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mtscout6

mtscout6 commented Dec 21, 2016

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jeanettehead

jeanettehead commented Dec 21, 2016

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mtscout6

mtscout6 commented Dec 21, 2016

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Dec 21, 2016

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jeanettehead

jeanettehead commented Jan 9, 2017

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mtscout6

mtscout6 commented Jan 11, 2017

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jeanettehead

jeanettehead commented Jan 11, 2017

Copy link
Copy Markdown
Contributor Author

I'm OK with that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mtscout6 commented Jan 11, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Jan 11, 2017

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Jan 11, 2017

Copy link
Copy Markdown
Member

e.g. npm run release -- minor

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Jan 11, 2017

Copy link
Copy Markdown
Member

Looks like this broke the build 😢

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Jan 11, 2017

Copy link
Copy Markdown
Member

@jeanettehead

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mtscout6

mtscout6 commented Jan 11, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@clayreimann

clayreimann commented Jan 11, 2017

Copy link
Copy Markdown
Member

Ahh, I guess I misunderstood.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miyajan miyajan mentioned this pull request Sep 18, 2017
chore: fix broken tests #474
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@jeanettehead @clayreimann @mtscout6
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.