Skip to content

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

Open
JamBalaya56562 wants to merge 2 commits into
google:masterfrom
JamBalaya56562:refactor/3644-pull-request-comment-value-params
Open

refactor!: Rename EditComment to UpdateComment on PullRequestsService, split review comment request bodies, and pass by value#4493
JamBalaya56562 wants to merge 2 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.53%. Comparing base (34cec4f) to head (b3a119b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4493   +/-   ##
=======================================
  Coverage   98.53%   98.53%           
=======================================
  Files         195      195           
  Lines       17691    17691           
=======================================
  Hits        17431    17431           
  Misses        260      260           

☔ 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

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). NeedsReview PR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants