refactor(github): get files from commits w/out PRs #825
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #825 +/- ##
==========================================
+ Coverage 93.68% 93.73% +0.05%
==========================================
Files 64 64
Lines 9067 9148 +81
Branches 964 928 -36
==========================================
+ Hits 8494 8575 +81
Misses 570 570
Partials 3 3
Continue to review full report at Codecov.
|
| while (moreFiles) { | ||
| const response = (await this.request( | ||
| 'GET /repos/{owner}/{repo}/commits/{ref}{?page}', | ||
| {owner: this.owner, repo: this.repo, ref, page} |
There was a problem hiding this comment.
see code comment in feedback
src/github.ts
Outdated
| break; | ||
| } | ||
| files.push(...commitFiles.map(f => f.filename ?? '')); | ||
| if (commitFiles.length < 300) { |
There was a problem hiding this comment.
I found this slightly confusing, does the API contract say that it will return up to 300 files in the first request?
Mind just adding a comment that links to the API docs describing the limits of this API? I think there's also an upper-bound limit of 3000 files (unlikely for many people to bump into).
| @@ -504,7 +504,7 @@ export class Manifest { | |||
| if (fromSha === undefined) { | |||
| fromSha = (await this.getConfigJson())['bootstrap-sha']; | |||
| } | |||
| return this.gh.commitsSinceSha(fromSha); | |||
| return this.gh.commitsSinceShaRest(fromSha); | |||
There was a problem hiding this comment.
Is there any food reason for us to keep commitsSinceSha?
Perhaps just open a tracking ticket to get rid of it? (I understand not wanting to do it as part of this work).
commitsSinceShaRest queries the REST API to get files associated directly with a history of commits. This solves the problem that commitsWithFiles has with processing a multi-commit PR: it ends up assigning all the files for the PR to every commit in the PR. I have not tested it but I think it could replace commitsWithFiles ( but not commitsWithLabels since there are no PRs as part of this query).
joeldodge79
force-pushed
the
github-commits-since-sha-rest
branch
from
9d952db to
e96564b
Compare
| // TODO: it is more robust to follow the link.next headers (in case | ||
| // GitHub ever changes the pattern) OR use ocktokit pagination for this | ||
| // endpoint when https://git.io/JmVll is addressed. |
There was a problem hiding this comment.
I know this is kind of hacky/rookie... but do you mind if I just add a tracking issue to harden/future-proof it (maybe add to #830)?
commitsSinceShaRest queries the REST API to get files associated directly
with a history of commits. This solves the problem that commitsWithFiles
has with processing a multi-commit PR: it ends up assigning all the
files for the PR to every commit in the PR.
I have not tested it but I think it could replace commitsWithFiles (
but not commitsWithLabels since there are no PRs as part of this query).