Refactor codebase to use value parameters instead of pointers where appropriate

[alexandear]

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. CreateRepositoryRelease with 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)

[rupokghosh]

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]

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 🤔

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]

Thank you for the clarifications! I will gladly take on this issue :))

[mwriter]

use pointer parameters (e.g. *RepositoryRelease) even when the function doesn't mutate the input

Are there many structures inside the golang itself where methods are separated according to this principle?
Especially if the api specification may change and the structures themselves may become more complex.
Personally, it always seems to me a bad approach to pass any complex structures by value.
The current implementation worked very fast and with minimal memory consumption. I'm not at all sure that these changes will benefit if multiple repositories with multiple releases are being processed at the same time.