-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check PR changed files count directly from get pull request endpoint #875
check PR changed files count directly from get pull request endpoint #875
Conversation
Thanks for your interest in palantir/policy-bot, @dblinkhorn! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this! I think we can simplify the implementation by taking advantage of the fact that we already load the pull request object when creating the pull.Context
pull/github.go
Outdated
return ghc.files, nil | ||
} | ||
|
||
func (ghc *GitHubContext) ChangedFilesCount() (int, error) { | ||
pullRequest, _, err := ghc.client.PullRequests.Get(ghc.ctx, ghc.owner, ghc.repo, ghc.number) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shouldn't be necessary to add a new method to get the changed files from the pull request object because we load the pull request as part of creating the GitHubContext
object. Instead, I think we need to extend the stored object to include this field:
- Modify the
v4PullRequestStruct
to include a newChangedFiles
field - Update
Locator.IsComplete
to return false if theChangedFiles
field is missing or empty - Update
Locator.toV4
to se the newChangedFiles
field from thegithub.PullRequest
object in theLocator
After that, you should be able to use ghc.pr.ChangedFiles
to reference this in the ChangedFiles
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions on the simpler implementation. I pushed a new commit with these changes.
pull/context.go
Outdated
@@ -255,4 +255,5 @@ type Body struct { | |||
CreatedAt time.Time | |||
Author string | |||
LastEditedAt time.Time | |||
ChangedFiles int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it looks like nothing uses this field after calling the Body()
method, I think should remove it for now and only add it in if it is required at some later point. The same thing would apply to the modification to the v4PullRequestWithEditedAt
struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I have removed both fields from these structs.
Thanks for the fix! This should be available in the |
Implements a fix for the checking of the files changed count for a PR. Now the check will be against the git pull request endpoint
changed_files
property rather than the list of files returned from the list pull request files endpoint.