[release] Add GitHubClient: pull request support#61767
Conversation
|
Reviews in this chain: |
|
There was a problem hiding this comment.
Code Review
This pull request adds functionality to fetch a single pull request from GitHub by its number. The changes include new dataclasses for the pull request and its author, a new method in GitHubRepo to perform the fetch, and comprehensive tests covering the happy path, error cases, and authentication headers. The implementation is clear and well-tested. I have one suggestion to improve the code structure by encapsulating the object creation logic within the dataclasses themselves, which will enhance maintainability.
release/ray_release/github_client.py
Outdated
| def get_pull(self, number: int) -> GitHubPull: | ||
| """Return a single pull request by number.""" | ||
| data = self._client._get(f"/repos/{self.full_name}/pulls/{number}") | ||
| return GitHubPull( | ||
| number=data["number"], | ||
| user=GitHubPullUser(login=data["user"]["login"]), | ||
| ) |
There was a problem hiding this comment.
For better separation of concerns and maintainability, consider moving the logic for creating GitHubPull and GitHubPullUser instances from the API response dictionary into factory class methods on the dataclasses themselves.
This would look something like this:
@dataclass
class GitHubPullUser:
login: str
@classmethod
def from_dict(cls, data: dict) -> "GitHubPullUser":
return cls(login=data["login"])
@dataclass
class GitHubPull:
number: int
user: GitHubPullUser
@classmethod
def from_dict(cls, data: dict) -> "GitHubPull":
return cls(
number=data["number"],
user=GitHubPullUser.from_dict(data["user"])
)
# In GitHubRepo:
def get_pull(self, number: int) -> GitHubPull:
"""Return a single pull request by number."""
data = self._client._get(f"/repos/{self.full_name}/pulls/{number}")
return GitHubPull.from_dict(data)This pattern makes your dataclasses responsible for their own construction from the raw API data, which is a cleaner design that will be easier to maintain and extend.
0a40bf8 to
40ba2ee
Compare
40ba2ee to
d7447e9
Compare
bbd8bef to
7a62c7f
Compare
d7447e9 to
ae5a4a0
Compare
Add GitHubPullUser, GitHubPull dataclasses and GitHubRepo.get_pull(). Tests cover successful fetch, 404 error, and auth header verification. Topic: github-client-pulls Relative: github-client Signed-off-by: andrew <andrew@anyscale.com>
ae5a4a0 to
75b1f4b
Compare
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| @responses.activate |
There was a problem hiding this comment.
this test fixture is really neat~
Add GitHubPullUser, GitHubPull dataclasses and GitHubRepo.get_pull(). Tests cover successful fetch, 404 error, and auth header verification. Signed-off-by: andrew <andrew@anyscale.com>
Add GitHubPullUser, GitHubPull dataclasses and GitHubRepo.get_pull().
Tests cover successful fetch, 404 error, and auth header verification.
Topic: github-client-pulls
Relative: github-client
Signed-off-by: andrew andrew@anyscale.com