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

Added ApiOption overloads to methods on I(Observable)RepositoryPagesClient #1304

Merged

Conversation

shiftkey
Copy link
Member

Fixes #1178

Supersedes #1213 and addresses merge conflict

@shiftkey
Copy link
Member Author

@ryangribble can I get a 👍 here before I merge this in?

@ryangribble
Copy link
Contributor

Looks pretty good, although I didn't see integration tests that actually test the pagination... Is that because there isn't a suitable repo that has enough responses?

@shiftkey
Copy link
Member Author

@ryangribble it's because anything related to Pages requires the account to be an administrator of the repository: #1263

@ryangribble
Copy link
Contributor

Yeah I saw the existing test was skipped, I guess I was just questioning whether there should still be the pagination test written, then also skipped?

The code changes look fine though so 👍 if you aren't concerned about the missing test

@shiftkey
Copy link
Member Author

@ryangribble good point. I'll add the test so we don't forget it and then close this out...

@shiftkey shiftkey merged commit 289cae5 into octokit:master May 20, 2016
@shiftkey shiftkey deleted the shiftkey-repository-pages-api-options branch May 20, 2016 11:02
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