Added PyGithub package to invoke github api calls#54
Conversation
1- Except get_file the rest use the package 2- get_file uses requests since iter_chunks helps with reading large files and using the package wasn't straightforward to handle large files Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
Replaced graphql api call with vanilla requests call since using the package doesn't have a value add Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
Currently, all changes are not being read by the action so fixing it by adding the ref with a depth of 0 to get all commits in the pr branch to checkout and run the tool Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
Fixed the action to checkout all changes to the poetry.lock file alone in pr and run the tool against the main branch Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
Detected 3 changes to dependencies in Poetry lockfileFrom base 5c4ab21 to target refs/pull/54/merge: Added pygithub (2.8.1) (3 added, 0 removed, 0 updated, 68 not changed) Generated by diff-poetry-lock 1.0.1.dev0 |
Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
colindean
left a comment
colindean
left a comment
There was a problem hiding this comment.
So, this shaves… 5 lines of code, but we basically gain some standardization and gets pagination for free where we weren't using it (and may not actually need it…).
How do you feel about this? Is it worth the switch?
| issue = self.repo.get_issue(int(self.s.pr_num)) | ||
| issue_comment = issue.get_comment(comment_id) | ||
| issue_comment.edit(f"{MAGIC_COMMENT_IDENTIFIER}{comment}") |
There was a problem hiding this comment.
thought: Hmmmmmmmmmmmm this is tripling the requests, eh?
I think we can use the data we already have to initialize an Issue and save the first and maybe also do it for IssueComment.
issue_comment = github.IssueComment(id=comment_id)but this might explicitly need a URL to be set, too. We have the info to build that URL…
issue_comment = github.IssueComment(
id=comment_id,
url=f"{self.s.api_url}/repos/{self.s.repository}/issues/comments/{comment_id}"
)could extract that URL building to a function…
def build_issue_comment_url(self, comment_id: int) -> str:
return f"{self.s.api_url}/repos/{self.s.repository}/issues/comments/{comment_id}"
issue_comment = github.IssueComment(
id=comment_id,
url=build_issue_comment_url(comment_id)
)This reduces the reliance on the library, which we're increasing drastically in this PR, but saves a few extra requests and thus a few tens of milliseconds of compute and API limits and whatnot. Probably worth it.
There was a problem hiding this comment.
Sure. Yes I'll look at customizing it
| issue = self.repo.get_issue(int(self.s.pr_num)) | ||
| issue_comment = issue.get_comment(comment_id) | ||
| issue_comment.delete() |
There was a problem hiding this comment.
thought: Same deal, and likely same deal in other places where it takes a few more HTTP requests to get what we need when using the library… when we already have some or all of the metadata that the library is going to get in those requests.
| class Headers(Enum): | ||
| """Enum for github api headers.""" | ||
|
|
||
| JSON = "application/vnd.github+json" | ||
| RAW = "application/vnd.github.raw" | ||
|
|
||
| def headers(self, token: str) -> dict[str, str]: | ||
| return {"Authorization": f"Bearer {token}", "Accept": self.value} |
There was a problem hiding this comment.
question: Is this still used? I think PyGitHub might provide this. Check out github/Auth.py, perhaps github.Token to wrap the token and github.Consts for the MIME types.
How we build the headers dict for the few requests we are managing… might still be this method.
There was a problem hiding this comment.
Yes the headers are used in the two get_file and resolve_commit_hashes that I didn't switch but yes I can take a look at Auth.py to reuse it
I felt the library wasn't that worth to switch over to because I feel we have to customize each use case we've either to reduce the network calls or just that it doesn't support some use cases yet like the two calls we make: get_file and resolve_commit_hashes which still uses requests We could still incorporate it for standardization though since if we decide to go with the lib, we can set it up once after customization and reap its benefits |
2 participants
1- Except get_file and resolve_commit_hashes the rest use the package
2- get_file uses requests since iter_chunks helps with reading large files and using the package wasn't straightforward to handle large files
3- resolve_commit_hashes uses requests because the package didn't give a value add
This handles #47