Repository navigation
Refactor codebase to use value parameters instead of pointers where appropriate #3644
Description
Many methods in the codebase, such as:
func (s *RepositoriesService) CreateRelease(ctx context.Context, owner, repo string, release *RepositoryRelease) (*RepositoryRelease, *Response, error)use pointer parameters (e.g. *RepositoryRelease) even when the function doesn't mutate the input. This appears to be a result of convention or copy-paste, not a performance-driven choice.
For instance, in CreateRelease, the RepositoryRelease struct is relatively large (~15+ fields), but most of its fields are pointers themselves. As such, passing it by value would have negligible performance impact in typical API usage. Moreover, value semantics would improve clarity, especially for required arguments that aren't modified.
To improve API design consistency and safety, we should:
-
Audit method signatures that accept pointer structs as input
-
Change them to accept values when:
-
The input is required (not optional)
-
The function does not modify the input
-
-
Optionally introduce dedicated input structs for specific operations (e.g.
CreateRepositoryReleasewith only the fields needed for creation)
Note: This would be a breaking change and should be handled carefully, possibly across multiple PRs.
Originally posted by @gmlewis in #3636 (comment)
Activity
[-]> I agree, that better is to use values here and elsewhere. But this is a large refactoring that break a lot.[/-][+]Refactor codebase to pass parameters by value instead of by pointer[/+][-]Refactor codebase to pass parameters by value instead of by pointer[/-][+]Refactor codebase to use value parameters instead of pointers where appropriate[/+]rupokghosh commentedon Jul 27, 2025
Does the issue need to be assigned or can I push a PR for this? I was wondering how i would be separating the PRs because changes would be all across the codebase 🤔
gmlewis commentedon Jul 27, 2025
Ideally, we prefer to assign owners to issues so that work does not get accidentally duplicated.
It's very frustrating to work on an issue and another PR pops up out of nowhere that does the same thing that you are working on.
As for separating it into multiple PRs, we prefer to keep PRs on the smaller side when possible to make them easier to review and to understand. Since this repo is divided into multiple "Service" areas, it might make sense to make a separate PR for each different GitHub service.
I'll assign the issue to you, @rupokghosh, since it sounds like you are interested in working on this. Thank you!
rupokghosh commentedon Jul 27, 2025
Thank you for the clarifications! I will gladly take on this issue :))
rupokghosh commentedon Aug 10, 2025
Hi, due to work constraints, I am not too sure if I can finish this issue by myself, or at least anytime soon so if anyone wants to continue the work that I started here, please feel free to go ahead!
gmlewis commentedon Aug 10, 2025
No problem, @rupokghosh. Thank you for letting us know.
This issue is now available for anyone else who would like to work on it. Please let us know, though, so we can assign it to you so that your work is not duplicated by someone else.
sarthakw7 commentedon Aug 15, 2025
Hi, I’d like to work on this issue. Could you please assign it to me?
Thanks!
gmlewis commentedon Aug 15, 2025
Thank you, @sarthakw7! It is yours.
43 remaining items
feat: Add DuplicateIssueID to IssueRequest
refactor!: Rename Issues.Edit to Issues.Update
refactor!: Use omitzero for IssueRequest Labels and Assignees
refactor!: Rename IssueRequest to UpdateIssueRequest
refactor!: Rename `EditLabel` to `UpdateLabel`
refactor!: Prefix label request types with `Issue` and align `Label` …
ManavSharma142 commentedon Aug 5, 2026
Hi! I'd be happy to help with this refactor. I noticed
UsersService.Editalso seems to match the criteria (required input, not mutated, mostly pointer fields). If nobody is working on theUsersService, I'd be happy to take it and open a PR.gmlewis commentedon Aug 5, 2026
Thank you, @ManavSharma142! As long as you coordinate all your PRs with both @JamBalaya56562 and @jvm986, then I'm happy to add you as a third contributor to this issue. The goal is that all 3 of you are aware of the PRs that you are currently working on such that no effort is duplicated and therefore wasted.
chore: Remove stale `paramcheck` allowlist entries
github: annotate initial request types with //meta:schema
kimjooyoon commentedon Oct 12, 2026
@jvm986 @JamBalaya56562 @ManavSharma142 — is
RepositoriesService.Createavailable for a small contribution to this migration?At
40091e2, it takes a required, non-mutated*Repositoryand copies the supported creation fields into the privatecreateRepoRequest. I propose introducing an exported creation request type passed by value, following the current required/optional field conventions, and updating the call sites and tests for the user and organization endpoints. This would cover only this method, not the whole Repositories service.I haven't started a patch and will wait for coordination to avoid overlapping your work. I am using AI assistance for this investigation and message; any patch would be disclosed and checked against the contribution guidelines.
55408suga commentedon Oct 13, 2026
Hi @JamBalaya56562 @jvm986 @ManavSharma142,
I'd like to help with this migration as my first contribution to go-github. Would it be okay for me to take
WorkflowsPermissionsOptand the three public methods that use it?RepositoriesService.UpdatePrivateRepoForkPRWorkflowSettingsActionsService.UpdatePrivateRepoForkPRWorkflowSettingsInOrganizationActionsService.UpdatePrivateRepoForkPRWorkflowSettingsInEnterpriseAt e754d2c, all three still take the request body by pointer. I propose passing it by value, renaming the input type to follow the Request naming convention, and updating the related tests, generated accessors, and linter exception.
I'd preserve the existing fields and JSON behavior, including the distinction between omitted optional booleans and explicit false values.
I checked the open PRs and didn't find overlapping changes, but wanted to check with you before starting. Does this scope sound reasonable, and is anyone already planning to work on it?
I used AI assistance to investigate the code . Thanks!
jvm986 commentedon Oct 13, 2026
@kimjooyoon I think that's available -- from my point of view you are free to open a PR.
@55408suga same for you, from my point of view feel free to grab those.
kimjooyoon commentedon Oct 13, 2026
Thanks @jvm986. PR #4616 is now ready for review and still covers only
RepositoriesService.Create, withCreateRepositoryRequestpassed by value and the user/org tests and callers updated. The repository's generation, formatting, test, and lint scripts passed locally, and the Google CLA check now passes. The upstream tests and lint workflows are awaiting maintainer approval.@JamBalaya56562 @ManavSharma142 — sharing the link here to keep the scope coordinated.