Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

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.

Signed-off-by: Byron Anderson <banderson@pivotal.io>
@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
ContributorAuthor

@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
ContributorAuthor

@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
ContributorAuthor

I'm OK with that.

@mtscout6mtscout6 merged commitf43d486 intogithub-tools:masterJan 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. Therelease 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.

@miyajanmiyajan mentioned this pull requestSep 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jeanettehead@clayreimann@mtscout6

[8]ページ先頭

©2009-2025 Movatter.jp