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

Add pagination support to GitHub.API.Installation.repositories #1162

Merged

Conversation

joshsmith
Copy link
Contributor

@joshsmith joshsmith commented Nov 6, 2017

What's in this PR?

Adds pagination support to GitHub.API.Installation.repositories.

Wasn't sure if a test was necessary given that it was failing until I got it working. Don't think we need to do extra testing for this to be paginating.

Any thoughts here?

References

Fixes #1108

@joshsmith
Copy link
Contributor Author

It looks like this is erroring due to the fact that get_all may not handle individual errors from the GitHub API well.

For example, the list returned seems to be:

[%CodeCorps.GitHub.APIError{documentation_url: nil, errors: nil,
  message: "{\"message\":\"Maximum number of login attempts exceeded. Please try again later.\",\"documentation_url\":\"https://developer.github.com/v3\"}",
  status_code: 404}]

And Installation.repositories does not indicate that this is an error, but instead tries to pass this off as a list of repos.

@begedin
Copy link
Contributor

begedin commented Nov 7, 2017

@joshsmith Took me a bit to get here, but I managed to write some behavior which should allow us to more easily recover from various potential issues during a paginated request.

There were a bunch of inconsistencies that needed fixing, as well as miscellaneous minor corrections I had to do to get to that point, but overall, this should increase stability.

Just as a minor tip, I found that looking at dialyzer errors, which we have quite a lot of, using mix dialyzer was helpful in identifying potential bugs. We should strive to resolve as much of those as we can and keep them to a minimum, but I don't think it should be our focus. Just something we do while we work on a feature or bug.

The commit message should clarify what I did but I'll also copy it here for convenience

  • added API.Errors namespace, to move other errors into
  • added API.Errors.PaginationError
  • rewrote some specs that were breaking contracts
  • standardized get_all/3 response to match request/5 response ({:ok, data} or {:error, error})
  • corrected several type definitions
  • added support for recovering from failed individual page requests during get_all/3
  • added support for recovering from failed initial head request during get_all/3
  • added support for marshalling a failed and successful head response (body is ""so poison was failing to decode)
  • rewrote ResultAggregator docs and argument names so they reflect the fact that it can be used for aggregating any standard response list, not just db commit
  • wrote tests to cover new behaviors

@joshsmith
Copy link
Contributor Author

This looks great to me. I can't approve since it's my PR, but will rebase and merge now. Great work!

@joshsmith joshsmith force-pushed the 1108-add-pagination-support-to-installation-repositories branch from b850c0a to 0a5d957 Compare November 7, 2017 21:40
joshsmith and others added 2 commits November 7, 2017 13:45
- added API.Errors namespace, to move other errors into
- added API.Errors.PaginationError
- rewrote some specs that were breaking contracts
- standardized get_all response to match request response
- corrected several type definitions
- added support for recovering from failed individual page requests during get_all
- added support for recovering from failed initial head request during get_all
- added support for marshalling a failed and succesful head response (body is "")
- rewrote ResultAggregator docs and argument names so they reflect the fact that it can be used for aggregating any standard response list, not just db commit
- wrote tests to cover new behaviors
@joshsmith joshsmith force-pushed the 1108-add-pagination-support-to-installation-repositories branch from 0a5d957 to 84c5435 Compare November 7, 2017 21:45
@joshsmith joshsmith merged commit 6d9c919 into develop Nov 7, 2017
@joshsmith joshsmith deleted the 1108-add-pagination-support-to-installation-repositories branch November 7, 2017 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants