- Notifications
You must be signed in to change notification settings - Fork3.1k
feat: add reviewers parameter to UpdatePullRequest#285
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Uh oh!
There was an error while loading.Please reload this page.
JoannaaKL commentedJun 24, 2025
Hi@MayorFaj, can you please update the pr so we can get it merged? :) |
…ation- go run ./cmd/github-mcp-server generate-docs
MayorFaj commentedJul 7, 2025
@JoannaaKL the PR has been updated |
deivyrene commentedJul 17, 2025
Could you update again because is out-of-date? Great PR! |
pkg/github/pullrequests.go Outdated
| } | ||
| // If no updates and no reviewers, return error | ||
| if!updateNeeded&&len(reviewers)==0 { |
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.
Can we move this check before the line 316?
pkg/github/pullrequests.go Outdated
| }else { | ||
| // If no update needed, just get the current PR | ||
| varghResp*github.Response | ||
| pr,ghResp,err=client.PullRequests.Get(ctx,owner,repo,pullNumber) |
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.
We we need this else? If no updates are needed and no new reviewers were requested we will just return an error (please see comment below).
JoannaaKL left a comment
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.
Looks good! I'd update the part where we're fetching the pull request - I don't think we need that :)
* initial impl of pull request draft state update* appease linter* update README* add nosec* fixed err return type for json marshalling* add gql test
…b#775)* make repo optional, and default to .github when not provided. improve tool description* autogen* update tests* small copy paste error fixes
…or reviewer updates
MayorFaj commentedJul 29, 2025
@JoannaaKL , if you get the chance could you please review this PR again so it can be merged asap. Has changes get merged to main, I am having to resolve conflicts, which is time consuming |
mattdholloway left a comment
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.
Mostly looks good, just a few changes in reference to the recent draft updates to the PR tool
pkg/github/pullrequests_test.go Outdated
| iftc.expectedPR.Title!=nil { | ||
| assert.Equal(t,*tc.expectedPR.Title,*returnedPR.Title) | ||
| } | ||
| iftc.expectedPR.Body!=nil { | ||
| assert.Equal(t,*tc.expectedPR.Body,*returnedPR.Body) | ||
| } | ||
| iftc.expectedPR.State!=nil { | ||
| assert.Equal(t,*tc.expectedPR.State,*returnedPR.State) | ||
| } | ||
| iftc.expectedPR.Base!=nil&&tc.expectedPR.Base.Ref!=nil { | ||
| assert.NotNil(t,returnedPR.Base) | ||
| assert.Equal(t,*tc.expectedPR.Base.Ref,*returnedPR.Base.Ref) | ||
| } | ||
| iftc.expectedPR.MaintainerCanModify!=nil { | ||
| assert.Equal(t,*tc.expectedPR.MaintainerCanModify,*returnedPR.MaintainerCanModify) | ||
| } |
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.
This change is out of scope for this PR
pkg/github/pullrequests_test.go Outdated
| mock.WithRequestMatch( | ||
| mock.GetReposPullsByOwnerByRepoByPullNumber, | ||
| mockNoUpdatePR, | ||
| ), |
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.
Getting the PR here is not needed
pkg/github/pullrequests_test.go Outdated
| mockNoUpdatePR:=&github.PullRequest{ | ||
| Number:github.Ptr(42), | ||
| Title:github.Ptr("Test PR"), | ||
| State:github.Ptr("open"), | ||
| } |
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.
This can be removed as well due to the referencing test not being required
MayorFaj commentedJul 31, 2025
@mattdholloway Thanks for the feedback, changes updated, unused mock test removed |
| } | ||
| // If no updates, no draft change, and no reviewers, return error early | ||
| if!restUpdateNeeded&&!draftProvided&&len(reviewers)==0 { |
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.
We could simplify it to!(restUpdateNeeded || draftProvided || len(reviewers) != 0)
45e90ae intogithub:mainUh oh!
There was an error while loading.Please reload this page.
* feat: add reviewers parameter to UpdatePullRequest and update tests* Update pullrequests.go* feat: enhance update pull request functionality with reviewers support* update README to clarify optional reviewers parameter in API documentation- go run ./cmd/github-mcp-server generate-docs* feat: enhance UpdatePullRequest to return early if no updates or reviewers are provided* Add updating draft state to `update_pull_request` tool (github#774)* initial impl of pull request draft state update* appease linter* update README* add nosec* fixed err return type for json marshalling* add gql test* Add support for org-level discussions in list_discussions tool (github#775)* make repo optional, and default to .github when not provided. improve tool description* autogen* update tests* small copy paste error fixes* refactor: streamline UpdatePullRequest logic and enhance test cases for reviewer updates* refactor: remove redundant draft update tests and streamline UpdatePullRequest logic* test: add unit tests for updating pull request draft state* refactor: simplify UpdatePullRequest tests by removing unused mock data---------Co-authored-by: Matt Holloway <mattdholloway@github.com>Co-authored-by: Tommaso Moro <37270480+tommaso-moro@users.noreply.github.com>
This pull request introduces the ability to request reviewers when updating a pull request. The changes include updates to the
UpdatePullRequestfunction to handle reviewers, modifications to the test cases to cover the new functionality, and updates to the documentation.Enhancements to
UpdatePullRequestfunction:UpdatePullRequestfunction. (pkg/github/pullrequests.go)[1][2][3]pkg/github/pullrequests.go)Documentation updates:
README.mdto include the newreviewersparameter in the documentation for updating pull requests. (README.md)Test case updates:
pkg/github/pullrequests_test.go)[1][2][3][4][5]Closes:#259