Make Repository.compare().commits return paginated list (#2882)

doc/changes.rst

@@ -4,6 +4,29 @@ Change log
 Stable versions
 ~~~~~~~~~~~~~~~
 
+Version 2.2.0 ()
+-----------------------------------
+
+Breaking Changes
+^^^^^^^^^^^^^^^^
+
+* The ``github.Comparison.Comparison`` instance returned by ``Repository.compare`` provides a ``commits``
+  property that used to return a ``list[github.Commit.Commit]``, which has now been changed
+  to ``PaginatedList[github.Commit.Commit]``. This breaks user code that assumes a ``list``:
+
+.. code-block:: python
+
+    commits = repo.compare("v0.6", "v0.7").commits
+    no_of_commits = len(commits)
+
+This will raise a ``TypeError: object of type 'PaginatedList' has no len()``, as the returned ``PaginatedList``
+does not support the ``len()`` method. Use the ``totalCount`` property instead:
+
+.. code-block:: python
+
+    commits = repo.compare("v0.6", "v0.7").commits
+    no_of_commits = commits.totalCount
+
 Version 2.1.1 (September 29, 2023)
 -----------------------------------
 

github/Comparison.py

@@ -40,6 +40,7 @@
 import github.Commit
 import github.File
 from github.GithubObject import Attribute, CompletableGithubObject, NotSet
+from github.PaginatedList import PaginatedList
 
 
 class Comparison(CompletableGithubObject):
@@ -51,7 +52,6 @@ def _initAttributes(self) -> None:
         self._ahead_by: Attribute[int] = NotSet
         self._base_commit: Attribute[github.Commit.Commit] = NotSet
         self._behind_by: Attribute[int] = NotSet
-        self._commits: Attribute[list[github.Commit.Commit]] = NotSet
         self._diff_url: Attribute[str] = NotSet
         self._files: Attribute[list[github.File.File]] = NotSet
         self._html_url: Attribute[str] = NotSet
@@ -80,10 +80,21 @@ def behind_by(self) -> int:
         self._completeIfNotSet(self._behind_by)
         return self._behind_by.value
 
+    # This should be a method, but this used to be a property and cannot be changed without breaking user code
+    # TODO: remove @property on version 3
     @property
-    def commits(self) -> list[github.Commit.Commit]:
-        self._completeIfNotSet(self._commits)
-        return self._commits.value
+    def commits(self) -> PaginatedList[github.Commit.Commit]:
+        return PaginatedList(
+            github.Commit.Commit,
+            self._requester,
+            self.url,
+            {},
+            None,
+            "commits",
+            "total_commits",
+            self.raw_data,
+            self.raw_headers,
+        )
 
     @property
     def diff_url(self) -> str:
@@ -137,8 +148,6 @@ def _useAttributes(self, attributes: dict[str, Any]) -> None:
             self._base_commit = self._makeClassAttribute(github.Commit.Commit, attributes["base_commit"])
         if "behind_by" in attributes:  # pragma no branch
             self._behind_by = self._makeIntAttribute(attributes["behind_by"])
-        if "commits" in attributes:  # pragma no branch
-            self._commits = self._makeListOfClassesAttribute(github.Commit.Commit, attributes["commits"])
         if "diff_url" in attributes:  # pragma no branch
             self._diff_url = self._makeStringAttribute(attributes["diff_url"])
         if "files" in attributes:  # pragma no branch

github/EnterpriseConsumedLicenses.py

@@ -87,8 +87,8 @@ def get_users(self) -> PaginatedList[NamedEnterpriseUser]:
             url_parameters,
             None,
             "users",
-            self.raw_data,
-            self.raw_headers,
+            firstData=self.raw_data,
+            firstHeaders=self.raw_headers,
         )
 
     def _useAttributes(self, attributes: Dict[str, Any]) -> None:

github/PaginatedList.py

@@ -152,6 +152,7 @@ def __init__(
         firstParams: Any,
         headers: Optional[Dict[str, str]] = None,
         list_item: str = "items",
+        total_count_item: str = "total_count",
         firstData: Optional[Any] = None,
         firstHeaders: Optional[Dict[str, Union[str, int]]] = None,
         attributesTransformer: Optional[Callable[[Dict[str, Any]], Dict[str, Any]]] = None,
@@ -164,6 +165,7 @@ def __init__(
         self.__nextParams = firstParams or {}
         self.__headers = headers
         self.__list_item = list_item
+        self.__total_count_item = total_count_item
         if self.__requester.per_page != 30:
             self.__nextParams["per_page"] = self.__requester.per_page
         self._reversed = False
@@ -255,7 +257,7 @@ def _getPage(self, data: Any, headers: Dict[str, Any]) -> List[T]:
                 self.__nextUrl = links["next"]
         self.__nextParams = None
         if self.__list_item in data:
-            self.__totalCount = data.get("total_count")
+            self.__totalCount = data.get(self.__total_count_item)
             data = data[self.__list_item]
         content = [
             self.__contentClass(self.__requester, headers, self._transformAttributes(element), completed=False)

github/Repository.py

@@ -1105,7 +1105,7 @@ def remove_invitation(self, invite_id: int) -> None:
 
     def compare(self, base: str, head: str) -> Comparison:
         """
-        :calls: `GET /repos/{owner}/{repo}/compare/{base...:head} <https://docs.github.com/en/rest/reference/repos#commits>`_
+        :calls: `GET /repos/{owner}/{repo}/compare/{base...:head} <https://docs.github.com/en/rest/commits/commits#compare-two-commits>`_
         :param base: string
         :param head: string
         :rtype: :class:`github.Comparison.Comparison`
@@ -1114,7 +1114,11 @@ def compare(self, base: str, head: str) -> Comparison:
         assert isinstance(head, str), head
         base = urllib.parse.quote(base)
         head = urllib.parse.quote(head)
-        headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/compare/{base}...{head}")
+        # the compare API has a per_page default of 250, which is different to Consts.DEFAULT_PER_PAGE
+        per_page = self._requester.per_page if self._requester.per_page != Consts.DEFAULT_PER_PAGE else 250
+        # only with page=1 we get the pagination headers for the commits element
+        params = {"page": 1, "per_page": per_page}
+        headers, data = self._requester.requestJsonAndCheck("GET", f"{self.url}/compare/{base}...{head}", params)
         return github.Comparison.Comparison(self._requester, headers, data, completed=True)
 
     def create_autolink(

tests/ReplayData/ExposeAllAttributes.testAllClasses.txt

@@ -299,7 +299,7 @@ https
 GET
 api.github.com
 None
-/repos/jacquev6/PyGithub/compare/master...develop
+/repos/jacquev6/PyGithub/compare/master...develop?page=1&per_page=250
 {'Authorization': 'Basic login_and_password_removed', 'User-Agent': 'PyGithub/Python'}
 None
 200

tests/ReplayData/Repository.testCompare.txt

@@ -2,7 +2,7 @@ https
 GET
 api.github.com
 None
-/repos/jacquev6/PyGithub/compare/v0.6...v0.7
+/repos/jacquev6/PyGithub/compare/v0.6...v0.7?page=1&per_page=250
 {'Authorization': 'Basic login_and_password_removed', 'User-Agent': 'PyGithub/Python'}
 None
 200

tests/Repository.py

@@ -704,6 +704,40 @@ def testCompare(self):
             ],
         )
 
+    def testCompareCommitPagination(self):
+        gh = github.Github(
+            auth=self.oauth_token,
+            per_page=4,
+            retry=self.retry,
+            pool_size=self.pool_size,
+            seconds_between_requests=self.seconds_between_requests,
+            seconds_between_writes=self.seconds_between_writes,
+        )
+        repo = gh.get_repo("PyGithub/PyGithub")
+        comparison = repo.compare("v1.54", "v1.54.1")
+        self.assertEqual(comparison.status, "ahead")
+        self.assertEqual(comparison.ahead_by, 10)
+        self.assertEqual(comparison.behind_by, 0)
+        self.assertEqual(comparison.total_commits, 10)
+        self.assertEqual(len(comparison.files), 228)
+        self.assertEqual(comparison.commits.totalCount, 10)
+        self.assertListKeyEqual(
+            comparison.commits,
+            lambda c: c.sha,
+            [
+                "fab682a5ccfc275c31ec37f1f541254c7bd780f3",
+                "9ee3afb1716c559a0b3b44e097c05f4b14ae2ab8",
+                "a806b5233f6423e0f8dacc4d04b6d81a72689bed",
+                "63e4fae997a9a5dc8c2b56907c87c565537bb28f",
+                "82c349ce3e1c556531110753831b3133334c19b7",
+                "2432cffd3b2f1a8e0b6b96d69b3dd4ded148a9f7",
+                "e113e37de1ec687c68337d777f3629251b35ab28",
+                "f299699ccd75910593d5ddf7cc6212f70c5c28b1",
+                "31a1c007808a4205bdae691385d2627c561e69ed",
+                "34d097ce473601624722b90fc5d0396011dd3acb",
+            ],
+        )
+
     def testGetComments(self):
         self.assertListKeyEqual(
             self.repo.get_comments(),