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

Ability to specify per_page for PullRequest.Files? #2552

Closed
Gamer025 opened this issue Aug 25, 2022 · 5 comments · Fixed by #2553
Closed

Ability to specify per_page for PullRequest.Files? #2552

Gamer025 opened this issue Aug 25, 2022 · 5 comments · Fixed by #2553

Comments

@Gamer025
Copy link
Contributor

Is it somehow possible to specify the per_page parameter when calling PullRequest.Files?
By default Github only returns 30 files per page but could return up to 100.
This means when retrieving a list of modified files on a large PR a lot of unneeded API calls are being made.

Apparently other GetAll methods allow you to pass an ApiOptions parameter but it doesn't look like PullRequest.Files has an overload for that.

Alternatively would it be possible for PullRequest.Files to include per_page set to 100 by default?
I don't see any reason why one wouldn't want that method to use as little API calls as possible.

@timrogers
Copy link
Contributor

@Gamer025 👋🏻 Hi there! Just to confirm, are you referring to this method?

If so, I can see that that REST API does support the per_page parameter.

Would you be open to creating a PR to add that? I think it would be better for Octokit.NET not to override the API's default per_page, but you could at least make it customisable.

@Gamer025
Copy link
Contributor Author

Hello @timrogers
Yes im referring to the method you linked.
I took a quick look at the code and as far as I can tell one probably only needs to add an overloaded Files method that takes in an ApiOptions object?
That object can then simply be passed on to ApiConnection.GetAll

@timrogers
Copy link
Contributor

That sounds like it will work to me! I'd welcome a PR.

@Gamer025
Copy link
Contributor Author

I submitted a PR that adds ApiOptions to the Files method.
I tested it in a local project and everything worked correctly and one could now query up to 100 files per page:
image
I had to change some of the unit test mocking because calling PullRequestsClient.Files without ApiOptions now calls the overloaded method version with ApiOptions set to None.

@timrogers
Copy link
Contributor

Thanks for taking the time to submit the PR, #2553! We'll get that reviewed.

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 a pull request may close this issue.

2 participants