Support new "Delete Reactions" endpoints #1451
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
Thank you, @wesleimp!
I have been trying to figure out if the *_number fields should be int instead of int64, but I'm not finding any other place in this repo where these fields are already represented except in IssueComment where the ID is an int64, so I'm thinking we can leave these.
So the only other thing that needs changing is to revert the changes to go.mod and go.sum, please.
Then we should be ready for a second LGTM before merging.
|
@martinssipenko - if you feel like reviewing and approving, your help would be greatly appreciated! |
github/reactions.go
Outdated
| // DeleteTeamDiscussionReactionByTeamIDAndOrgID deletes the reaction to a team discussion by organization ID and team ID. | ||
| // | ||
| // GitHub API docs: https://developer.github.com/v3/reactions/#delete-team-discussion-reaction | ||
| func (s *ReactionsService) DeleteTeamDiscussionReactionByTeamIDAndOrgID(ctx context.Context, orgID, teamID, discussionNumber, reactionID int64) (*Response, error) { |
There was a problem hiding this comment.
The method is described as "ByTeamIDAndOrgID", but the parameters are in the opposite order: orgID first, teamID second.
Having orgID be first makes sense, given it's a higher level piece of information. It's consistent with specifying repoID first, issueID second.
What do you think about renaming this method (and others like it) to be consistent with the parameters, i.e., "ByOrgIDAndTeamID"?
(Optional comment.)
There was a problem hiding this comment.
Another idea here is to use a simpler "ByID" suffix for all these methods that use IDs instead of repo/owner/org names.
That is what's used by many other existing endpoints:
$ go doc -all github.com/google/go-github/github | grep ByID | wc -l
55
There was a problem hiding this comment.
I agree, consistency would be good.
Codecov Report
@@ Coverage Diff @@
## master #1451 +/- ##
==========================================
+ Coverage 68.07% 68.16% +0.09%
==========================================
Files 92 94 +2
Lines 8307 8419 +112
==========================================
+ Hits 5655 5739 +84
- Misses 1795 1819 +24
- Partials 857 861 +4
Continue to review full report at Codecov.
|
|
OK, I think this PR is ready for merging and would be good to get into the repo before we make release If we could please get a second LGTM on it, that would be great. Thank you. |
There was a problem hiding this comment.
I apologize that I didn't catch this before, but I'm just realizing that in other places in our repo, whenever we have a *Number (as opposed to a *ID), we use int instead of int64.
So for consistency, I'm thinking that we should change all issueNumber, discussionNumber, and commentNumber arguments to be of type int instead of type int64 (and leave all the *ID types to be int64).
|
@wesleimp it would be great to have this merged in order to unblock v30 |
Closes #1442