Skip to content

refactor!: Rename EditComment to UpdateComment on IssuesService, and pass a new IssueCommentRequest by value - #4444

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

refactor!: Rename EditComment to UpdateComment on IssuesService, and pass a new IssueCommentRequest by value#4444
JamBalaya56562 wants to merge 2 commits into
google:masterfrom
JamBalaya56562:refactor/3644-issue-comment-value-params

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Continues the request-body-by-value work in #3644, this time for the issue comment endpoints on IssuesService.

CreateComment and EditComment reused the 11-field IssueComment response type as their request bodies, but per the docs both endpoints accept exactly one parameter, body, and it's required in both. The other ten fields (id, user, reactions, created_at, …) are server-generated. The clearest evidence is in the code itself — EditComment's doc comment had to warn:

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

The new request type makes that warning unnecessary by construction:

type IssueCommentRequest struct {
	Body string `json:"body"`
}

Unlike the milestone split in #4438, the create and update schemas here are identical, so a single shared request type is used rather than a create/update pair (same reasoning as HookConfig in #4360). Body is a non-pointer string since it's required in both operations.

EditComment is renamed to UpdateComment to match the docs operation name, Update an issue comment (same convention as #4438/#4400). Both methods now take the body by value, and IssueComment is removed from the body-allowed-pointer-types allowlist. The IssueComment response type itself is unchanged.

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

Updates #3644

BREAKING CHANGE: IssuesService.CreateComment now takes a new IssueCommentRequest (with non-pointer Body) by value, and IssuesService.EditComment is renamed to UpdateComment and takes the same IssueCommentRequest by value, instead of *IssueComment.

cc @jvm986 — second item in the Issues service; as before, happy to coordinate whenever you resume.

…, and pass a new `IssueCommentRequest` by value

CreateComment and EditComment reused the 11-field IssueComment response
type as their request bodies, but both endpoints accept exactly one
parameter, body, and it is required in both schemas. EditComment's doc
comment even had to warn "A non-nil comment.Body must be provided. Other
comment fields should be left nil" — the new shared IssueCommentRequest
makes that warning unnecessary by construction.

Since the create and update schemas are identical, a single shared request
type is used rather than a split. EditComment is renamed to UpdateComment
to match the docs operation name. The IssueComment response type stays
unchanged, and its entry is removed from the .golangci.yml allowlist.

BREAKING CHANGE: IssuesService.CreateComment now takes a new IssueCommentRequest (with non-pointer Body) by value, and IssuesService.EditComment is renamed to UpdateComment and takes the same IssueCommentRequest by value, instead of *IssueComment.
@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 8, 2026
@gmlewis

gmlewis commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Congratulations, @JamBalaya56562 on PR #4444 !!! 😂

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.55%. Comparing base (848675f) to head (96e0bc1).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4444   +/-   ##
=======================================
  Coverage   97.55%   97.55%           
=======================================
  Files         194      194           
  Lines       19892    19892           
=======================================
  Hits        19406    19406           
  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 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 the repo before merging.

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

@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.

Let's also update IssueComment struct.

// IssueComment represents a comment left on an issue.
type IssueComment struct {
	ID        *int64     `json:"id,omitempty"`
	NodeID    *string    `json:"node_id,omitempty"`
	Body      *string    `json:"body,omitempty"`
	User      *User      `json:"user,omitempty"`
	Reactions *Reactions `json:"reactions,omitempty"`
	CreatedAt *Timestamp `json:"created_at,omitempty"`
	UpdatedAt *Timestamp `json:"updated_at,omitempty"`
	// AuthorAssociation is the comment author's relationship to the issue's repository.
	// Possible values are "COLLABORATOR", "CONTRIBUTOR", "FIRST_TIMER", "FIRST_TIME_CONTRIBUTOR", "MEMBER", "OWNER", or "NONE".
	//
	// Deprecated: GitHub will remove this field from Events API payloads on October 7, 2025.
	// Use the Issue Comments REST API endpoint to retrieve this information.
	// See: https://docs.github.com/rest/issues/comments?apiVersion=2022-11-28#get-an-issue-comment
	AuthorAssociation *string `json:"author_association,omitempty"`
	URL               *string `json:"url,omitempty"`
	HTMLURL           *string `json:"html_url,omitempty"`
	IssueURL          *string `json:"issue_url,omitempty"`
}

It has few missing fields - performed_via_github_app, pin and minimized
https://docs.github.com/en/rest/issues/comments?apiVersion=2022-11-28#update-an-issue-comment

…ment`

The issue-comment response schema includes performed_via_github_app, pin
and minimized, which were missing from the Go struct. pin and minimized
are modeled with the new PinnedIssueComment and MinimizedIssueComment
types matching their schemas.
@JamBalaya56562

Copy link
Copy Markdown
Contributor Author

It has few missing fields - performed_via_github_app, pin and minimized

Thanks @Not-Dhananjay-Mishra — added in 96e0bc1. I verified all three against the issue-comment response schema:

  • PerformedViaGithubApp *App — same shape as the existing five uses of performed_via_github_app in the codebase (Issue, IssueEvent, Timeline, …).
  • Pin *PinnedIssueComment — new type with PinnedAt *Timestamp / PinnedBy *User, matching the "Pinned Issue Comment" schema.
  • Minimized *MinimizedIssueComment — new type with Reason *string, matching the "Minimized Issue Comment" schema.

All three are nullable in the response, so they're pointers with omitempty, and the generated accessors are covered at 100%.

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.

3 participants