octocrab icon indicating copy to clipboard operation
octocrab copied to clipboard

Improve Commits API

Open cloud303-cholden opened this issue 1 year ago • 7 comments

After some research, a possible solution to a discussion I started (link) is to list commits and send a get request for each one in order to get its additions and deletions. It looks like the only way to make this request is with octocrab::Octocrab::get. Would you be open to a PR that adds the "Get a commit" API (link)? It looks like the natural place to add it would be octocrab::commits::CommitHandler::get. Maybe it would also be useful to have a method for octocrab::commits::CommitHandler::list -> octocrab::commits::ListCommitsBuilder as well?

cloud303-cholden avatar Jun 17 '23 18:06 cloud303-cholden

Thank you for your issue! Yes adding that API would be appreciated. As for where it should go, I would look up the octokit JS API and see where they put it

XAMPPRocky avatar Jun 17 '23 18:06 XAMPPRocky

Understood! I'll fork the repo and get started on it.

cloud303-cholden avatar Jun 17 '23 18:06 cloud303-cholden

It looks like getCommit is under the repos API (link), similar to listCommits. I'm curious whether you prefer using repos or commits, because it looks like createCommitComment is also under repos (link), whereas your crate has it under commits at octocrab::commits::CommitHandler::create_comment. The GitHub docs suggest that the /commits endpoints are distinct from /repos, but they all ultimately fall under the/repos endpoint. ~~If we put this API under repos, I'm guessing this would require a new GetRepoCommit model, since the response spec is different than the RepoCommit one.~~

cloud303-cholden avatar Jun 17 '23 23:06 cloud303-cholden

Ah yeah, I think it should go under commits, ultimately it’s very similar to repos because it does require you to specify a repo.

XAMPPRocky avatar Jun 18 '23 10:06 XAMPPRocky

So, I spent a few hours yesterday figuring out why the additions and deletions fields weren't populated. I used the PullRequestHandler::list method.

Reading this issue, am I correct that some (all?) of the optional fields in PullRequest are only populated when you do a call to fetch the information for that individual Pull Request?

That's good to know. Is this documented somewhere, and I just missed that detail?

JeanMertz avatar Aug 16 '23 12:08 JeanMertz

I just verified that this is indeed the case using:

gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/OWNER/REPO/pulls/PULL_NUMBER

Without PULL_NUMBER, many details are missing from the listed pull requests.

JeanMertz avatar Aug 16 '23 12:08 JeanMertz

That's good to know. Is this documented somewhere, and I just missed that detail?

I don't know if it's documented anywhere in the GitHub API, but we should probably add a note to the list method stating it.

XAMPPRocky avatar Aug 16 '23 14:08 XAMPPRocky