Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename "id" parameter to "commentID" in comment-related methods of IssuesService. #885

Closed
dmitshur opened this issue Mar 29, 2018 · 0 comments
Closed

Rename "id" parameter to "commentID" in comment-related methods of IssuesService. #885

dmitshur opened this issue Mar 29, 2018 · 0 comments
Labels
enhancement

Comments

@dmitshur
Copy link
Member

dmitshur commented Mar 29, 2018 •
edited
Loading

The endpoints that touch comments in IssuesService are very different from ones that touch issues. Comment-related endpoints take the comment ID (an int64 now), while issue-related endpoints use the issue number (an int):

IssuesService.Get       (ctx context.Context, owner string, repo string, number int) (*Issue, *Response, error)
IssuesService.GetComment(ctx context.Context, owner string, repo string, id int64)   (*IssueComment, *Response, error)

Using the name id for the parameter can be misleading. People can think it refers to the issue ID.

This problem is exacerbated in the EditComment endpoints because they take an id int64 and the *github.IssueComment itself also contains an ID int64 (but setting it has no effect; it's there for the Get/List operations, not Edit).

GistsService sets a good example by calling the parameter commentID in its GetComment method:

GistsService.GetComment(ctx context.Context, gistID string, commentID int64) (*GistComment, *Response, error)

I think we should rename id to commentID in IssuesService to make it more clear.

(I've also noticed a similar issue in PullRequestsService and its comment-related methods, but I'll make that a separate issue.)
@dmitshur dmitshur self-assigned this Mar 29, 2018
dmitshur added a commit that referenced this issue Mar 29, 2018
@dmitshur
Rename id parameter to commentID in IssuesService comment methods.
3e68bbc 
Also document which IssueComment fields need to be set in EditComment
method.

This should help improve clarity of the API, and reduce the chance to
mix up the wrong IDs.

Resolves #885.
@dmitshur dmitshur mentioned this issue Mar 29, 2018
Merged
@dmitshur dmitshur removed their assignment Mar 29, 2018
@dmitshur dmitshur changed the title Rename "id" parameter to "commentID" in IssuesService. Rename "id" parameter to "commentID" in comment-related methods of IssuesService. Mar 29, 2018
dmitshur added a commit that referenced this issue Mar 29, 2018
@dmitshur
Rename id parameter to commentID in IssuesService comment methods. (#886
4377977 
)

Also document which IssueComment fields need to be set in EditComment
method.

This should help improve clarity of the API, and reduce the chance to
mix up the issue vs comment IDs.

Resolves #885.
dmitshur added a commit that referenced this issue Mar 30, 2018
@dmitshur
Change number int to commentID int64 in PRsService comment methods.
787b5d0 
This is a breaking API change that is a part of issue #597. It should've
been applied earlier, but we missed it because the parameter was
misleadingly named number rather than id or commentID.

Rename the parameter to commentID to make it more clear that it's the
comment ID and not the PR ID nor PR number. This should help improve
clarity of the API, and reduce the chance to mix up the IDs.

Also document which PullRequestComment fields need to be set in
EditComment method.

Similar to #886.
Updates #885.
Updates #597.
This was referenced Mar 30, 2018
Merged
Closed
dmitshur added a commit that referenced this issue Apr 2, 2018
@dmitshur
Change number int to commentID int64 in PRsService comment methods. (#…
e5e3945 
…888)

This is a breaking API change that is a part of issue #597. It should've
been applied earlier, but we missed it because the parameter was
misleadingly named number rather than id or commentID.

Rename the parameter to commentID to make it more clear that it's the
comment ID and not the PR ID nor PR number. This should help improve
clarity of the API, and reduce the chance to mix up the IDs.

Also document which PullRequestComment fields need to be set in
EditComment method.

Similar to #886.
Updates #885.
Updates #597.
nbareil pushed a commit to nbareil/go-github that referenced this issue May 1, 2018
@dmitshur
Rename id parameter to commentID in IssuesService comment methods. (g…
fc17d76 
…oogle#886)

Also document which IssueComment fields need to be set in EditComment
method.

This should help improve clarity of the API, and reduce the chance to
mix up the issue vs comment IDs.

Resolves google#885.
nbareil pushed a commit to nbareil/go-github that referenced this issue May 1, 2018
@dmitshur
Change number int to commentID int64 in PRsService comment methods. (g…
580d873 
…oogle#888)

This is a breaking API change that is a part of issue google#597. It should've
been applied earlier, but we missed it because the parameter was
misleadingly named number rather than id or commentID.

Rename the parameter to commentID to make it more clear that it's the
comment ID and not the PR ID nor PR number. This should help improve
clarity of the API, and reduce the chance to mix up the IDs.

Also document which PullRequestComment fields need to be set in
EditComment method.

Similar to google#886.
Updates google#885.
Updates google#597.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dmitshur