Skip to content

refactor!: Rename EditComment to UpdateComment on PullRequestsService, split review comment request bodies, and pass by value - #4493

Merged
gmlewis merged 3 commits into
google:masterfrom
JamBalaya56562:refactor/3644-pull-request-comment-value-params
Aug 28, 2026
Merged

refactor!: Rename EditComment to UpdateComment on PullRequestsService, split review comment request bodies, and pass by value#4493
gmlewis merged 3 commits into
google:masterfrom
JamBalaya56562:refactor/3644-pull-request-comment-value-params

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Continues the request-body-by-value work in #3644, this time for the review comment endpoints on PullRequestsService — the pulls counterpart of #4444.

Commit 1 (refactor!:)CreateComment and EditComment reused the 27-field PullRequestComment response type as their request bodies, and EditComment's doc comment had to warn:

A non-nil comment.Body must be provided. Other comment fields should be left nil.

Per the docs, the two schemas differ, so the body is split (same approach as #4382/#4401/#4438):

Field Create (POST) Update (PATCH)
body required required (only parameter)
commit_id, path required
line, start_line, start_side conditionally required ("required unless …") → stay pointers
side, in_reply_to, subject_type optional
position optional, deprecated → kept with a // Deprecated: note

EditComment is renamed to UpdateComment to match the docs operation name (same convention as #4444). CreateCommentInReplyTo already builds its own minimal body internally and is unchanged. PullRequestComment is removed from the body-allowed-pointer-types allowlist.

Commit 2 (feat:) — while cross-checking the response schema (prompted by the review feedback on #4444 and #4477, where missing response fields were caught by reviewers), I found three properties missing from PullRequestComment and added them up front: BodyHTML, BodyText, and Links (_links, modeled with a new PullRequestCommentLinks type reusing the existing PRLink).

Verified with go build ./..., go vet -tags integration ./test/integration/, gofmt, the full ./github/ test suite (CreateComment/UpdateComment and the generated accessors at 100%), and custom-gcl (no paramcheck findings after removing the allowlist entry).

Updates #3644

BREAKING CHANGE: PullRequestsService.CreateComment now takes a new CreatePullRequestCommentRequest (with non-pointer Body, CommitID and Path) by value, and PullRequestsService.EditComment is renamed to UpdateComment and takes a new UpdatePullRequestCommentRequest by value, instead of *PullRequestComment.

cc @jvm986 — flagging for #3644 coordination; this is the pulls comments type, so no overlap with your recent work.

…rvice`, split review comment request bodies, and pass by value

CreateComment and EditComment reused the 27-field PullRequestComment
response type as their request bodies, and EditComment's doc comment had
to warn "A non-nil comment.Body must be provided. Other comment fields
should be left nil."

The create and update schemas differ — body, commit_id and path are
unconditionally required on create while update takes only body — so the
body is split into CreatePullRequestCommentRequest (line/start_line/
start_side stay pointers as they are only conditionally required, and the
deprecated position parameter is kept with a deprecation note) and
UpdatePullRequestCommentRequest.

EditComment is renamed to UpdateComment to match the docs operation name.
CreateCommentInReplyTo already builds its own minimal body and is
unchanged. The PullRequestComment response type stays unchanged, and its
entry is removed from the .golangci.yml allowlist.

BREAKING CHANGE: PullRequestsService.CreateComment now takes a new CreatePullRequestCommentRequest (with non-pointer Body, CommitID and Path) by value, and PullRequestsService.EditComment is renamed to UpdateComment and takes a new UpdatePullRequestCommentRequest by value, instead of *PullRequestComment.
The pull-request review comment response schema includes body_html,
body_text and _links, which were missing from the Go struct. _links is
modeled with the new PullRequestCommentLinks type reusing the existing
PRLink hypermedia link type.
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.52%. Comparing base (c48dbf5) to head (2f071c2).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4493   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files         195      195           
  Lines       17745    17745           
=======================================
  Hits        17484    17484           
  Misses        261      261           

☔ 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 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 Aug 26, 2026

@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

@gmlewis

gmlewis commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

@JamBalaya56562 - do you want to resolve the recent conflicts (probably from #4481 ) or would you like me to?
(I'm fine either way.)

@JamBalaya56562

JamBalaya56562 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Done — merged master into this branch and re-ran the generators (2f071c2). The conflicts were the #4481 Ptr->new conversions colliding with the request-type changes in pulls_comments_test.go plus the generated files; all tests and custom-gcl pass locally.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 28, 2026
@gmlewis
gmlewis merged commit 95224ef into google:master Aug 28, 2026
15 checks passed
@JamBalaya56562
JamBalaya56562 deleted the refactor/3644-pull-request-comment-value-params branch August 28, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants