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

repo.compare_commits doesn't use pagination #1137

Closed
amaccormack-lumira opened this issue Apr 13, 2023 · 4 comments
Closed

repo.compare_commits doesn't use pagination #1137

amaccormack-lumira opened this issue Apr 13, 2023 · 4 comments

Comments

@amaccormack-lumira
Copy link
Contributor

Since 2021, github API supports pagination on Comparison results: https://github.blog/changelog/2021-03-22-compare-rest-api-now-supports-pagination/

but this package still gives a list of 250, not an iterator that would use pagination, meaning diffs are incomplete.

@sigmavirus24
Copy link
Owner

Would be happy to review a PR that fixed this

@amaccormack-lumira
Copy link
Contributor Author

amaccormack-lumira commented Apr 13, 2023

Yep, got a solution working on my machine, but never submitted a PR to anyone else's project before! Give me a minute and I'll try.

basically, changing comparison.py:

    def _update_attributes(self, compare):
        self._api = compare["url"]
        self.ahead_by = compare["ahead_by"]
        self.base_commit = commit.ShortCommit(compare["base_commit"], self)
        self.behind_by = compare["behind_by"]
        self.commits = compare["commits"]
        self.total_commits = compare["total_commits"]
        if self.commits:
            if self.total_commits>len(self.commits):
                self.commits=self._iter(-1, self._api, commit.ShortCommit, list_key="commits")
            else:
                self.commits = [
                    commit.ShortCommit(com, self) for com in self.commits
                ]

@sigmavirus24
Copy link
Owner

Just always do _iter. Inconsistent return types will bite users and confuse them.

@amaccormack-lumira
Copy link
Contributor Author

Fair enough, although by default github API returns 250 items with the original call, so trying to save some bandwidth

amaccormack-lumira added a commit to andrewmaccormack/github3.py that referenced this issue Apr 13, 2023
based on feedback from sigmavirus24 in issue sigmavirus24#1137
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

No branches or pull requests

2 participants