Merge pull request #749 from omgjlk/refactor-pulls-670

Implement PullRequest refactor

github3/events.py

@@ -50,6 +50,24 @@ class EventOrganization(GitHubCore):
         return self._instance_or_null(orgs.Organization, json)
+class EventPullRequest(GitHubCore):
+    """The class that represents the pr information returned in Events."""
+
+    def _update_attributes(self, pull):
+        self.id = pull['id']
+        self.number = pull['number']
+        self.state = pull['state']
+        self.title = pull['title']
+        self.locked = pull['locked']
+        self._api = self.url = pull['url']
+
+    def to_pull(self):
+        """Retrieve a full PullRequest object for this EventPullRequest."""
+        from . import pulls
+        json = self._json(self._get(self.url), 200)
+        return self._instance_or_null(pulls.PullRequest, json)
+
+
 class Event(GitHubCore):
     """The :class:`Event <Event>` object. It structures and handles the data
@@ -161,19 +179,18 @@ def _member(payload, session):
 def _pullreqev(payload, session):
-    from .pulls import PullRequest
     if payload.get('pull_request'):
-        payload['pull_request'] = PullRequest(payload['pull_request'],
-                                              session)
+        payload['pull_request'] = EventPullRequest(payload['pull_request'],
+                                                   session)
     return payload
 def _pullreqcomm(payload, session):
-    from .pulls import PullRequest, ReviewComment
+    from .pulls import ReviewComment
     # Transform the Pull Request attribute
     pull = payload.get('pull_request')
     if pull:
-        payload['pull_request'] = PullRequest(pull, session)
+        payload['pull_request'] = EventPullRequest(pull, session)
     # Transform the Comment attribute
     comment = payload.get('comment')

github3/pulls.py

 # -*- coding: utf-8 -*-
-"""
-github3.pulls
-=============
-
-This module contains all the classes relating to pull requests.
-
-"""
+"""This module contains all the classes relating to pull requests."""
 from __future__ import unicode_literals
 from json import dumps
@@ -24,7 +18,10 @@ from .repos.contents import Contents
 class PullDestination(models.GitHubCore):
     """The :class:`PullDestination <PullDestination>` object.
-    See also: http://developer.github.com/v3/pulls/#get-a-single-pull-request
+    Please see GitHub's `PullRequest Documentation`_ for more information.
+
+    .. _PullRequest Documentation:
+        http://developer.github.com/v3/pulls/#get-a-single-pull-request
     """
     def __init__(self, dest, direction):
@@ -58,7 +55,10 @@ class PullFile(models.GitHubCore):
     """The :class:`PullFile <PullFile>` object.
-    See also: http://developer.github.com/v3/pulls/#list-pull-requests-files
+    Please see GitHub's `PR Files Documentation`_ for more information.
+
+    .. _PR Files Documentation:
+        http://developer.github.com/v3/pulls/#list-pull-requests-files
     """
     def _update_attributes(self, pfile):
@@ -104,84 +104,43 @@ class PullFile(models.GitHubCore):
         return self._instance_or_null(Contents, json)
-class PullRequest(models.GitHubCore):
+class _PullRequest(models.GitHubCore):
     """The :class:`PullRequest <PullRequest>` object.
-    Two pull request instances can be checked like so::
-
-        p1 == p2
-        p1 != p2
-
-    And is equivalent to::
-        p1.id == p2.id
-        p1.id != p2.id
-
-    See also: http://developer.github.com/v3/pulls/
+    Please see GitHub's `PullRequests Documentation`_ for more information.
+    .. _PullRequests Documentation:
+        http://developer.github.com/v3/pulls/
     """
     def _update_attributes(self, pull):
-        self._api = self._get_attribute(pull, 'url')
+        self._api = pull['url']
         #: Base of the merge
-        self.base = self._class_attribute(
-            pull, 'base', PullDestination, 'Base'
-        )
+        self.base = PullDestination(pull['base'], 'Base')
         #: Body of the pull request message
-        self.body = self._get_attribute(pull, 'body')
+        self.body = pull['body']
         #: Body of the pull request as HTML
-        self.body_html = self._get_attribute(pull, 'body_html')
+        self.body_html = pull['body_html']
         #: Body of the pull request as plain text
-        self.body_text = self._get_attribute(pull, 'body_text')
-
-        #: Number of additions on this pull request
-        self.additions_count = self._get_attribute(pull, 'additions')
-
-        #: Number of deletions on this pull request
-        self.deletions_count = self._get_attribute(pull, 'deletions')
+        self.body_text = pull['body_text']
         #: datetime object representing when the pull was closed
         self.closed_at = self._strptime_attribute(pull, 'closed_at')
-        #: Number of comments
-        self.comments_count = self._get_attribute(pull, 'comments')
-
-        #: Comments url (not a template)
-        self.comments_url = self._get_attribute(pull, 'comments_url')
-
-        #: Number of commits
-        self.commits_count = self._get_attribute(pull, 'commits')
-
-        #: GitHub.com url of commits in this pull request
-        self.commits_url = self._get_attribute(pull, 'commits_url')
-
         #: datetime object representing when the pull was created
         self.created_at = self._strptime_attribute(pull, 'created_at')
-        #: URL to view the diff associated with the pull
-        self.diff_url = self._get_attribute(pull, 'diff_url')
-
         #: The new head after the pull request
-        self.head = self._class_attribute(
-            pull, 'head', PullDestination, 'Head'
-        )
-
-        #: The URL of the pull request
-        self.html_url = self._get_attribute(pull, 'html_url')
+        self.head = PullDestination(pull['head'], 'Head')
         #: The unique id of the pull request
         self.id = self._get_attribute(pull, 'id')
-        #: The URL of the associated issue
-        self.issue_url = self._get_attribute(pull, 'issue_url')
-
-        #: Statuses URL
-        self.statuses_url = self._get_attribute(pull, 'statuses_url')
-
         #: Dictionary of _links. Changed in 1.0
         self.links = self._get_attribute(pull, '_links', {})
@@ -189,45 +148,16 @@ class PullRequest(models.GitHubCore):
         #: If merged, holds commit sha of the merge commit, squashed commit on
         #: the base branch or the commit that the base branch was updated to
         #: after rebasing the PR.
-        self.merge_commit_sha = self._get_attribute(pull, 'merge_commit_sha')
-
-        #: Boolean representing whether the pull request has been merged
-        self.merged = self._get_attribute(pull, 'merged')
+        self.merge_commit_sha = pull['merge_commit_sha']
         #: datetime object representing when the pull was merged
         self.merged_at = self._strptime_attribute(pull, 'merged_at')
-        #: Whether the pull is deemed mergeable by GitHub
-        self.mergeable = self._get_attribute(pull, 'mergeable', False)
-
-        #: Whether it would be a clean merge or not
-        self.mergeable_state = self._get_attribute(pull, 'mergeable_state')
-
-        #: :class:`User <github3.users.User>` who merged this pull
-        self.merged_by = self._class_attribute(
-            pull, 'merged_by', users.ShortUser, self,
-        )
-
         #: Number of the pull/issue on the repository
-        self.number = self._get_attribute(pull, 'number')
-
-        #: The URL of the patch
-        self.patch_url = self._get_attribute(pull, 'patch_url')
+        self.number = pull['number']
         #: Review comment URL Template. Expands with ``number``
-        self.review_comment_url = self._class_attribute(
-            pull, 'review_comment_url', URITemplate
-        )
-
-        #: Number of review comments on the pull request
-        self.review_comments_count = self._get_attribute(
-            pull, 'review_comments'
-        )
-
-        #: GitHub.com url for review comments (not a template)
-        self.review_comments_url = self._get_attribute(
-            pull, 'review_comments_url'
-        )
+        self.review_comment_url = URITemplate(pull['review_comment_url'])
         #: Returns ('owner', 'repository') this issue was filed on.
         self.repository = self.base
@@ -235,24 +165,30 @@ class PullRequest(models.GitHubCore):
             self.repository = self.base.repo
         #: The state of the pull
-        self.state = self._get_attribute(pull, 'state')
+        self.state = pull['state']
         #: The title of the request
-        self.title = self._get_attribute(pull, 'title')
+        self.title = pull['title']
         #: datetime object representing the last time the object was changed
         self.updated_at = self._strptime_attribute(pull, 'updated_at')
-        #: :class:`User <github3.users.User>` object representing the creator
-        #: of the pull request
-        self.user = self._class_attribute(pull, 'user', users.ShortUser, self)
-        #: :class:`User <github3.users.User>` object representing the assignee
-        #: of the pull request
+        #: :class:`User <github3.users.ShortUser>` object representing the
+        #:  creator of the pull request
+        self.user = users.ShortUser(pull['user'])
+        # This is only present if the PR has been assigned.
+        #: :class:`User <github3.users.ShortUser>` object representing the
+        #: assignee of the pull request
         self.assignee = self._class_attribute(
             pull, 'assignee', users.ShortUser, self,
         )
+        for urltype in ['comments_url', 'commits_url', 'diff_url',
+                        'html_url', 'issue_url', 'statuses_url',
+                        'patch_url', 'review_comments_url']:
+            setattr(self, urltype, pull[urltype])
+
     def _repr(self):
         return '<Pull Request [#{0}]>'.format(self.number)
@@ -452,11 +388,80 @@ class PullRequest(models.GitHubCore):
         return False
+class ShortPullRequest(_PullRequest):
+    """Object for the shortened representation of a PullRequest
+
+    GitHub's API returns different amounts of information about prs based
+    upon how that information is retrieved. Often times, when iterating over
+    several prs, GitHub will return less information. To provide a clear
+    distinction between the types of prs, github3.py uses different classes
+    with different sets of attributes.
+
+    .. versionadded:: 1.0.0
+    """
+
+    pass
+
+
+class PullRequest(_PullRequest):
+    """Object for the full representation of a PullRequest.
+
+    GitHub's API returns different amounts of information about prs based
+    upon how that information is retrieved. This object exists to represent
+    the full amount of information returned for a specific pr. For example,
+    you would receive this class when calling
+    :meth:`~github3.github.GitHub.pull_request`. To provide a clear
+    distinction between the types of prs, github3.py uses different classes
+    with different sets of attributes.
+
+    .. versionchanged:: 1.0.0
+    """
+
+    def _update_attributes(self, pull):
+        super(PullRequest, self)._update_attributes(pull)
+
+        #: Number of additions on this pull request
+        self.additions_count = pull['additions']
+
+        #: Number of deletions on this pull request
+        self.deletions_count = pull['deletions']
+
+        #: Number of comments
+        self.comments_count = pull['comments']
+
+        #: Number of commits
+        self.commits_count = pull['commits']
+
+        #: Boolean representing whether the pull request has been merged
+        self.merged = pull['merged']
+
+        # This can be True, False, or None(Null). None is when the
+        # mergeability is still being computed. We default to False
+        # in that case.
+        #: Whether the pull is deemed mergeable by GitHub
+        self.mergeable = self._get_attribute(pull, 'mergeable', False)
+
+        #: Whether it would be a clean merge or not
+        self.mergeable_state = pull['mergeable_state']
+
+        # This may? be None(Null) while mergeability is being determined
+        #: :class:`User <github3.users.User>` who merged this pull
+        self.merged_by = self._class_attribute(
+            pull, 'merged_by', users.ShortUser, self,
+        )
+
+        #: Number of review comments on the pull request
+        self.review_comments_count = pull['review_comments']
+
+
 class PullReview(models.GitHubCore):
     """The :class:`PullReview <PullReview>` object.
-    See also: https://developer.github.com/v3/pulls/reviews/
+    Please see GitHub's `PullReview Documentation`_ for more information.
+
+    .. _PullReview Documentation:
+        https://developer.github.com/v3/pulls/reviews/
     """
     def _update_attributes(self, preview):
@@ -493,19 +498,10 @@ class ReviewComment(models.BaseComment):
     """The :class:`ReviewComment <ReviewComment>` object.
-    This is used to represent comments on pull requests.
-
-    Two comment instances can be checked like so::
-
-        c1 == c2
-        c1 != c2
-
-    And is equivalent to::
-
-        c1.id == c2.id
-        c1.id != c2.id
-    See also: http://developer.github.com/v3/pulls/comments/
+    Please see GitHub's `Pull Comments Documentation`_ for more information.
+    .. _Pull Comments Documentation:
+        http://developer.github.com/v3/pulls/comments/
     """
     def _update_attributes(self, comment):

github3/repos/repo.py

@@ -27,7 +27,7 @@ from ..licenses import License
 from ..models import GitHubCore
 from ..notifications import Subscription, Thread
 from ..projects import Project
-from ..pulls import PullRequest
+from ..pulls import ShortPullRequest, PullRequest
 from ..utils import stream_response_to_file, timestamp_parameter
 from .branch import Branch
 from .comment import RepoComment
@@ -391,7 +391,7 @@ class Repository(GitHubCore):
         if data:
             url = self._build_url('pulls', base_url=self._api)
             json = self._json(self._post(url, data=data), 201)
-        return self._instance_or_null(PullRequest, json)
+        return self._instance_or_null(ShortPullRequest, json)
     @requires_auth
     def add_collaborator(self, username):
@@ -990,8 +990,8 @@ class Repository(GitHubCore):
         :param str base: (required), e.g., 'master'
         :param str head: (required), e.g., 'username:branch'
         :param str body: (optional), markdown formatted description
-        :returns: :class:`PullRequest <github3.pulls.PullRequest>` if
-            successful, else None
+        :returns: :class:`ShortPullRequest <github3.pulls.ShortPullRequest>`
+            if successful, else None
         """
         data = {'title': title, 'body': body, 'base': base,
                 'head': head}
@@ -1004,8 +1004,8 @@ class Repository(GitHubCore):
         :param int issue: (required), issue number
         :param str base: (required), e.g., 'master'
         :param str head: (required), e.g., 'username:branch'
-        :returns: :class:`PullRequest <github3.pulls.PullRequest>` if
-            successful, else None
+        :returns: :class:`ShortPullRequest <github3.pulls.ShortPullRequest>`
+            if successful, else None
         """
         if int(issue) > 0:
             data = {'issue': issue, 'base': base, 'head': head}
@@ -1842,7 +1842,7 @@ class Repository(GitHubCore):
         :param str etag: (optional), ETag from a previous request to the same
             endpoint
         :returns: generator of
-            :class:`PullRequest <github3.pulls.PullRequest>`\ s
+            :class:`ShortPullRequest <github3.pulls.ShortPullRequest>`\ s
         """
         url = self._build_url('pulls', base_url=self._api)
         params = {}
@@ -1854,7 +1854,7 @@ class Repository(GitHubCore):
         params.update(head=head, base=base, sort=sort, direction=direction)
         self._remove_none(params)
-        return self._iter(int(number), url, PullRequest, params, etag)
+        return self._iter(int(number), url, ShortPullRequest, params, etag)
     def readme(self):
         """Get the README for this repository.

tests/integration/test_repos_repo.py

@@ -430,7 +430,7 @@ class TestRepository(helper.IntegrationHelper):
                 body='Migrated create_pull to tests/unit'
             )
-        assert isinstance(pull_request, github3.pulls.PullRequest)
+        assert isinstance(pull_request, github3.pulls.ShortPullRequest)
     def test_create_pull_from_issue(self):
         """
@@ -447,7 +447,7 @@ class TestRepository(helper.IntegrationHelper):
                 head='itsmemattchung:tests/migrate-repos',
             )
-        assert isinstance(pull_request, github3.pulls.PullRequest)
+        assert isinstance(pull_request, github3.pulls.ShortPullRequest)
     def test_create_release(self):
         """Test the ability to create a release on a repository."""
@@ -934,7 +934,7 @@ class TestRepository(helper.IntegrationHelper):
         assert len(pulls) > 0
         for pull in pulls:
-            assert isinstance(pull, github3.pulls.PullRequest)
+            assert isinstance(pull, github3.pulls.ShortPullRequest)
     def test_pull_requests_accepts_sort_and_direction(self):
         """Test that pull_requests now takes a sort parameter."""

tests/unit/test_events.py

@@ -96,7 +96,7 @@ class TestPayLoadHandlers(TestCase):
         pull_request = {'pull_request': get_pull_request_example_data()}
         github3.events._pullreqev(pull_request, None)
         assert isinstance(pull_request['pull_request'],
-                          github3.pulls.PullRequest)
+                          github3.events.EventPullRequest)
     def test_pullreqcomment(self):
         """Show that the event type is a PullRequestReviewCommentEvent."""

tests/unit/test_pulls.py

@@ -147,9 +147,9 @@ class TestPullRequest(helper.UnitHelper):
     def test_attributes(self):
         """Show that we extract attributes correctly."""
         assert self.instance.merge_commit_sha == \
-            'e5bd3914e2e596debea16f433f57875b5b90bcd6'
+            'f13731c44acf96f2e5d6f0080f54e09215e36248'
         assert not self.instance.merged
-        assert self.instance.mergeable
+        assert not self.instance.mergeable
 class TestPullRequestRequiresAuthentication(