refactor!: Refactor GistsService to use value parameters #3680
Conversation
gmlewis
changed the title
gmlewis
added
NeedsReview
gmlewis
changed the title
gmlewis
changed the title
There was a problem hiding this comment.
Thank you, @sarthakw7!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@alexandear or @stevehipwell - might you have time for a code review? Thank you!
There was a problem hiding this comment.
This PR looks good @sarthakw7 but I'd like to hear back from @gmlewis about the API design before approving.
| // Edit a gist. | ||
| // | ||
| // GitHub API docs: https://docs.github.com/rest/gists/gists#update-a-gist | ||
| // | ||
| //meta:operation PATCH /gists/{gist_id} | ||
| func (s *GistsService) Edit(ctx context.Context, id string, gist *Gist) (*Gist, *Response, error) { | ||
| func (s *GistsService) Edit(ctx context.Context, id string, gist UpdateGistRequest) (*Gist, *Response, error) { |
There was a problem hiding this comment.
| func (s *GistsService) Edit(ctx context.Context, id string, gist UpdateGistRequest) (*Gist, *Response, error) { | |
| func (s *GistsService) Update(ctx context.Context, id string, gist UpdateGistRequest) (*Gist, *Response, error) { |
I think the API consistency would be improved if we replaced Edit with Update and if we're changing the method signature that seems like a good time to make this change? @gmlewis what do you think?
There was a problem hiding this comment.
Yes, I agree. Thank you, @stevehipwell.
There was a problem hiding this comment.
I’ll go ahead and replace Edit → Update and EditComment → UpdateComment to keep things consistent
| func (s *GistsService) CreateFromGist(ctx context.Context, gist *Gist) (*Gist, *Response, error) { | ||
| var req CreateGistRequest | ||
|
|
||
| if gist != nil { | ||
| req = CreateGistRequest{ | ||
| Description: gist.Description, | ||
| Public: gist.Public, | ||
| Files: gist.Files, | ||
| } | ||
| } | ||
|
|
||
| return s.Create(ctx, req) | ||
| } |
There was a problem hiding this comment.
Why do we need this wrapper?
In this PR #3654 we didn't introduce any wrappers, but it breaks API in a similar way.
There was a problem hiding this comment.
I agree that we don't need the wrappers. I was trying to decide, but I think you are correct, @alexandear.
There was a problem hiding this comment.
Got it.
Since wrappers aren’t needed, I’ll go ahead and remove the four deprecated methods (CreateFromGist, EditFromGist, CreateCommentFromGistComment, and EditCommentFromGistComment)
BREAKING CHANGE:
GistsServicemethods now pass required params by-value instead of by-ref.Summary
This PR refactors the GistsService to use value parameters instead of pointer parameters where appropriate, addressing issue #3644. This is the first service in a planned series of PRs to improve API design consistency across the entire library.
Changes
New Input Structs
CreateGistRequest- Input for creating gists (3 fields)UpdateGistRequest- Input for updating gists (2 fields)CreateGistCommentRequest- Input for creating gist comments (1 field)UpdateGistCommentRequest- Input for updating gist comments (1 field)Refactored Methods (New API)
Create(ctx, CreateGistRequest)- Uses value parameterEdit(ctx, id, UpdateGistRequest)- Uses value parameterCreateComment(ctx, gistID, CreateGistCommentRequest)- Uses value parameterEditComment(ctx, gistID, commentID, UpdateGistCommentRequest)- Uses value parameterBackward Compatibility (Deprecated API)
CreateFromGist(ctx, *Gist)- Wrapper for backward compatibilityEditFromGist(ctx, id, *Gist)- Wrapper for backward compatibilityCreateCommentFromGistComment(ctx, gistID, *GistComment)- Wrapper for backward compatibilityEditCommentFromGistComment(ctx, gistID, commentID, *GistComment)- Wrapper for backward compatibility