Skip to content

refactor!: Rename PullRequestReviewDismissalRequest to PullRequestDismissReviewRequest, add PullRequestSubmitReviewRequest, and pass review request bodies by value - #4406

Merged
gmlewis merged 5 commits into
google:masterfrom
JamBalaya56562:refactor/3644-dismiss-review-value-params
Jul 31, 2026
Merged

Conversation

@JamBalaya56562

@JamBalaya56562 JamBalaya56562 commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Continues the request-body-by-value work in #3644, for the review dismissal and submission endpoints on PullRequestsService.

Dismiss a review

  • message is required, so Message becomes a non-pointer string (no omitempty), and DismissReview takes the body by value. The type is removed from the body-allowed-pointer-types allowlist.
  • Per review: the type is renamed to PullRequestDismissReviewRequest to match the method name, and the previously unmodeled optional event parameter is added as Event *string.

Submit a review

Per review, SubmitReview no longer reuses *PullRequestReviewRequest — whose node_id, commit_id and comments fields aren't part of the submit-a-review schema — and instead takes a new PullRequestSubmitReviewRequest by value, modeling that schema exactly: Event is required (non-pointer string), Body is optional (*string). CreateReview keeps the shared PullRequestReviewRequest, so its allowlist entry remains.

Verified with go build ./..., go vet -tags integration ./test/integration/, gofmt, the full ./github/ test suite, and custom-gcl (no paramcheck findings).

Updates #3644

BREAKING CHANGE: PullRequestReviewDismissalRequest is renamed to PullRequestDismissReviewRequest with non-pointer Message; PullRequestsService.DismissReview takes it by value; PullRequestsService.SubmitReview now takes a new PullRequestSubmitReviewRequest (non-pointer Event) by value instead of *PullRequestReviewRequest.

cc @jvm986 — flagging for #3644 coordination; this is in the pulls service, so it shouldn't overlap with the Issues work.

@JamBalaya56562
refactor!: Pass PullRequestReviewDismissalRequest by value
83c8680 
message is required by the dismiss-a-review endpoint, so `Message` becomes a
non-pointer string without omitempty, and `PullRequestsService.DismissReview`
now takes the request body by value. The type is removed from the
.golangci.yml allowlist.

BREAKING CHANGE: PullRequestReviewDismissalRequest.Message is now a non-pointer string, and PullRequestsService.DismissReview now takes the request body by value.
@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.54%. Comparing base (c86fe1b) to head (98e9721).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4406   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         194      194           
  Lines       19780    19780           
=======================================
  Hits        19294    19294           
  Misses        268      268           
  Partials      218      218

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gmlewis
gmlewis approved these changes Jul 24, 2026
View reviewed changes

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @JamBalaya56562!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

alexandear
alexandear reviewed Jul 24, 2026
View reviewed changes

@alexandear alexandear left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also fix the body in PullRequestsService.SubmitReview.

Comment thread github/pulls_reviews.go Outdated
alexandear
alexandear reviewed Jul 24, 2026
View reviewed changes
Comment thread github/pulls_reviews.go
@JamBalaya56562
refactor!: Rename PullRequestReviewDismissalRequest to `PullRequest…
8ed3f9c 
…DismissReviewRequest`

Aligns the request type name with the PullRequestsService.DismissReview
method name, as suggested in review.
@JamBalaya56562
feat: Add Event field to PullRequestDismissReviewRequest
7575960 
The dismiss-a-review endpoint accepts an optional event parameter
(can be "DISMISS") that was missing from the Go struct.
@JamBalaya56562
refactor!: Pass SubmitReview request body by value via new `PullReq…
8383de1 
…uestSubmitReviewRequest`

SubmitReview reused *PullRequestReviewRequest, whose node_id, commit_id and
comments fields are not part of the submit-a-review request schema. The new
PullRequestSubmitReviewRequest models that schema exactly: event is required
and becomes a non-pointer string, body stays optional.

CreateReview keeps the shared PullRequestReviewRequest, so its allowlist
entry remains.
@JamBalaya56562 JamBalaya56562 changed the title refactor!: Pass PullRequestReviewDismissalRequest by value refactor!: Rename PullRequestReviewDismissalRequest to PullRequestDismissReviewRequest, add PullRequestSubmitReviewRequest, and pass review request bodies by value Jul 24, 2026
@JamBalaya56562

JamBalaya56562 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Let's also fix the body in PullRequestsService.SubmitReview.

Done in 8383de1. Rather than passing the shared PullRequestReviewRequest by value, SubmitReview now takes a new PullRequestSubmitReviewRequest — the submit-a-review schema only has body (optional) and event (required), so the shared type's node_id/commit_id/comments don't belong there. Event is a non-pointer string accordingly. CreateReview keeps the shared type, so its allowlist entry remains.

PR title and description updated to cover all three changes.

Not-Dhananjay-Mishra
Not-Dhananjay-Mishra approved these changes Jul 31, 2026
View reviewed changes

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gmlewis

gmlewis commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Thank you, @Not-Dhananjay-Mishra!
@JamBalaya56562 - please merge the master branch into this PR and rerun step 4 of CONTRIBUTING.md to update the auto-generated files and push the changes to this PR. Then we should be ready to merge.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 31, 2026
@JamBalaya56562
Merge branch 'master' into refactor/3644-dismiss-review-value-params
98e9721 
The only conflicts were adjacent additions in the generated accessors
(PullRequestStack from master next to PullRequestSubmitReviewRequest from
this branch), resolved by regenerating them.
@JamBalaya56562

JamBalaya56562 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Done in 98e9721 — merged master in (no force-push) and re-ran the generators.

The only conflicts were in the generated accessors, where master's PullRequestStack additions landed right next to this branch's PullRequestSubmitReviewRequest ones, so regenerating produced the correct union. Re-running the generators afterwards is a no-op, and go build ./..., go vet -tags integration ./test/integration/ and the full ./github/ test suite are green locally.

@gmlewis

gmlewis commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Thank you, @JamBalaya56562, @alexandear, and @Not-Dhananjay-Mishra!
Merging.

@gmlewis
gmlewis merged commit 4d45b7c into google:master Jul 31, 2026
15 checks passed
@JamBalaya56562
JamBalaya56562 deleted the refactor/3644-dismiss-review-value-params branch July 31, 2026 18:39
@JamBalaya56562 JamBalaya56562 mentioned this pull request Jul 31, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes
@alexandear alexandear Awaiting requested review from alexandear
+1 more reviewer
@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Not-Dhananjay-Mishra approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JamBalaya56562 @gmlewis @alexandear @Not-Dhananjay-Mishra