Support preview Team Discussions API #889

parkhyukjun89 · 2018-04-03T19:28:06Z

Add a new service TeamsService to support API related to Organization Teams https://developer.github.com/v3/teams/
Add methods to support new preview API for team discussions https://developer.github.com/v3/teams/discussions/
Add methods to support new preview API for team discussion comments https://developer.github.com/v3/teams/discussion_comments/

Fixes #849
https://developer.github.com/changes/2018-02-07-team-discussions-api/#new-rest-api-endpoints

Second part of the change https://developer.github.com/changes/2018-02-07-team-discussions-api/#updates-to-the-teams-api-documentation will be done in a separate PR, once this PR is merged.

- Add a new service TeamsService to support API related to Organization Teams https://developer.github.com/v3/teams/ - Add methods to support new preview API for team discussions https://developer.github.com/v3/teams/discussions/ - Add methods to support new preview API for team discussion comments https://developer.github.com/v3/teams/discussion_comments/ Fixes #849 https://developer.github.com/changes/2018-02-07-team-discussions-api/

gmlewis

This is looking great, @parkhyukjun89... thank you!
I have a few minor points that would be nice to address, and then I want to take a second pass while looking more closely at the docs.

gmlewis · 2018-04-09T13:01:28Z

github/teams_discussion_comments.go

+	"fmt"
+)
+
+// DiscussionComment represents a GitHub dicussion in a team


nit: end comment with a period.

gmlewis · 2018-04-09T13:01:46Z

github/teams_discussion_comments.go

+}
+
+// DiscussionCommentListOptions specifies optional parameters to the
+// TeamServices.ListComments method


nit: end comment with a period.

gmlewis · 2018-04-09T13:05:20Z

github/teams_discussion_comments.go

+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussion_comments/#create-a-comment
+func (s *TeamsService) CreateComment(ctx context.Context, team, discussion int64, comment *DiscussionComment) (*DiscussionComment, *Response, error) {
+	u := fmt.Sprintf("teams/%v/discussions/%v/comments", team, discussion)


I'm thinking we might want to check for comment == nil here and return an error if so.

Added a nil check

gmlewis · 2018-04-09T13:05:59Z

github/teams.go

@@ -0,0 +1,7 @@
+package github
+
+// TeamsService provides access to the team related functions


nit: s/team related/team-related/

gmlewis · 2018-04-09T13:06:49Z

github/teams_discussion_comments.go

+// User is allowed to edit body of a comment only.
+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussion_comments/#edit-a-comment
+func (s *TeamsService) EditComment(ctx context.Context, team, discussion, id int64, comment *DiscussionComment) (*DiscussionComment, *Response, error) {


We might also want to check for comment == nil here as well.

Added a nil check

Similarly, what do you think of just making comment a value rather than pointer here as well?

gmlewis · 2018-04-09T13:09:03Z

github/teams_discussions.go

+}
+
+// DiscussionListOptions specifies optional parameters to the
+// TeamServices.ListDiscussions method


nit: end comment with a period.

gmlewis · 2018-04-09T13:10:54Z

github/teams_discussions.go

+type DiscussionListOptions struct {
+	// Sorts the discussion by the date they were created.
+	// Accepted values are asc and desc. Default is desc.
+	Direction string `url:"direction,omitempty"`


Do we also need to include ListOptions here? I haven't yet checked the docs.

doc mentions directions only https://developer.github.com/v3/teams/discussions/#parameters

gmlewis · 2018-04-09T13:11:45Z

github/teams_discussion_comments.go

+type DiscussionCommentListOptions struct {
+	// Sorts the discussion comments by the date they were created.
+	// Accepted values are asc and desc. Default is desc.
+	Direction string `url:"direction,omitempty"`


Do we also need to include ListOptions here? I haven't yet checked the docs.

doc mentions directions only https://developer.github.com/v3/teams/discussion_comments/#parameters

gmlewis · 2018-04-09T13:13:03Z

github/teams_discussions.go

+// Authenticated user must grant write:discussion scope.
+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussions/#create-a-discussion
+func (s *TeamsService) CreateDiscussion(ctx context.Context, team int64, discussion *TeamDiscussion) (*TeamDiscussion, *Response, error) {


We might want to check for discussion == nil here and return an error if so.

Added a nil check

gmlewis · 2018-04-09T13:13:37Z

github/teams_discussions.go

+// User is allowed to change Title and Body of a discussion only.
+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussions/#edit-a-discussion
+func (s *TeamsService) EditDiscussion(ctx context.Context, team, id int64, discussion *TeamDiscussion) (*TeamDiscussion, *Response, error) {


We might want to check for discussion == nil here and return an error if so.

Added a nil check

parkhyukjun89 · 2018-04-15T17:51:17Z

Travis build failed at 'go get' stage.

package golang.org/x/tools/cmd/cover: unrecognized import path "golang.org/x/tools/cmd/cover" (https fetch: Get https://golang.org/x/tools/cmd/cover?go-get=1: net/http: TLS handshake timeout)

Is there a way to kick off another build?

dmitshur · 2018-04-15T19:56:03Z

I've restarted the Travis build, it's passing now.

gmlewis

Thank you, @parkhyukjun89!
LGTM.
Awaiting second LGTM before merging.

gmlewis · 2018-04-18T15:42:17Z

It looks like merge conflicts need resolving, @parkhyukjun89. Either you can do that, or I can take a look at it later today.

parkhyukjun89 · 2018-04-19T05:36:22Z

Resolved merge conflict @gmlewis

dmitshur

This looks great overall, but there are a few API-related concerns I think we should pay attention to, and consider changing before merging.

dmitshur · 2018-04-19T23:36:11Z

github/teams_discussion_comments.go

+// Authenticated user must grant read:discussion scope.
+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussion_comments/#get-a-single-comment
+func (s *TeamsService) GetComment(ctx context.Context, team, discussion, id int64) (*DiscussionComment, *Response, error) {


team, discussion, id int64

I think we'll need to take care of this (here, and elsewhere).

From looking over the API, it does seem they use "number" instead of "id" to identify discussions and comments within said discussions.

However, I haven't actually used team discussions, so I can't confirm if this matches the reality.

From https://developer.github.com/v3/teams/discussion_comments/#get-a-single-comment, I'm seeing:

GET /teams/:team_id/discussions/:discussion_number/comments/:comment_number

That tells me that the first parameter is a team ID (which we want to use int64 for), second one is the discussion number (not ID), which we want to map to int, and the comment also seems to be a number rather than ID (hence int).

I suggest we follow that closely and therefore use:

teamID int64, discussionNumber, commentNumber int

Let me know what you think, or if you have any concerns.

Agreed. Discussion & comment numbers are incrementing numbers.

dmitshur · 2018-04-19T23:41:11Z

github/teams_discussion_comments.go

+func (s *TeamsService) CreateComment(ctx context.Context, team, discussion int64, comment *DiscussionComment) (*DiscussionComment, *Response, error) {
+	if comment == nil {
+		return nil, nil, errors.New("comment must be provided")
+	}


We did the if pull == nil change for existing endpoints where a pointer was already used (see PR #539), in order to avoid making a breaking API change.

Since this is a new API, I think we can simplify things and just make comment a value rather than pointer. After all, it's required.

It would be a little inconsistent with previous endpoints that use pointers, but I think it's a beneficial tradeoff. What do you think?

Understood. But, what if user decides to pass in an empty struct. Should that be considered an error and need to be guarded against?

But, what if user decides to pass in an empty struct. Should that be considered an error and need to be guarded against?

I think it's okay to let the GitHub API handle such an incorrect request (with a descriptive error message). We shouldn't, because it adds code, and it's not worth it (most people will not try to create an invalid comment, other than during testing). We don't try to catch empty or invalid input in all other endpoints either.

Making it a value rather than pointer is more about eliminating an unnecessary choice and making the API more intuitive to use. Since there's no reason to use a pointer (other than being consistent with prior unfortunate API choices, which isn't a strong enough reason I think), we shouldn't use it.

dmitshur · 2018-04-19T23:41:31Z

github/teams_discussion_comments.go

+// User is allowed to edit body of a comment only.
+//
+// GitHub API docs: https://developer.github.com/v3/teams/discussion_comments/#edit-a-comment
+func (s *TeamsService) EditComment(ctx context.Context, team, discussion, id int64, comment *DiscussionComment) (*DiscussionComment, *Response, error) {


Similarly, what do you think of just making comment a value rather than pointer here as well?

- remove use of pointers - use int for incrementing numbers

parkhyukjun89 · 2018-04-26T02:46:10Z

@shurcooL made changes accordingly

dmitshur

I think the changes are fantastic. The parameter names are very clear, and using values (over pointers) for mandatory parameters removes ambiguity. Thanks for doing that.

LGTM.

@gmlewis Do the changes look okay to you too?

gmlewis

Very nice! I like these changes a lot.
Thank you, @parkhyukjun89 and @shurcooL!
LGTM.
Merging.

dmitshur

I just spotted a few minor issues.

It should be easy to fix them, since this code has been merged very recently, there can’t be many users of it yet.

dmitshur · 2018-04-27T03:06:36Z

github/teams_discussion_comments.go

+	DiscussionURL *string    `json:"discussion_url,omitempty"`
+	HTMLURL       *string    `json:"html_url,omitempty"`
+	NodeID        *string    `json:"node_id,omitempty"`
+	Number        *int64     `json:"number,omitempty"`


I just noticed this, it’s not an ID, so it should be int. We use int type for all numbers other than IDs.

dmitshur · 2018-04-27T03:07:15Z

github/teams_discussions.go

+	Body          *string    `json:"body,omitempty"`
+	BodyHTML      *string    `json:"body_html,omitempty"`
+	BodyVersion   *string    `json:"body_version,omitempty"`
+	CommentsCount *int64     `json:"comments_count,omitempty"`


Also here, should be int.

dmitshur · 2018-04-27T03:07:32Z

github/teams_discussions.go

+	LastEditedAt  *Timestamp `json:"last_edited_at,omitempty"`
+	HTMLURL       *string    `json:"html_url,omitempty"`
+	NodeID        *string    `json:"node_id,omitempty"`
+	Number        *int64     `json:"number,omitempty"`


gmlewis · 2018-04-27T03:29:29Z

Great catches, @shurcooL! Thank you!

parkhyukjun89 · 2018-04-27T06:30:21Z

I will add those fixes in the next pr for https://developer.github.com/changes/2018-02-07-team-discussions-api/#updates-to-the-teams-api-documentation

Fixes google#849.

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Apr 3, 2018

gmlewis suggested changes Apr 9, 2018

View reviewed changes

code review changes

d6eb23b

gmlewis approved these changes Apr 18, 2018

View reviewed changes

merge upstream

fce90c0

dmitshur reviewed Apr 19, 2018

View reviewed changes

code review changes

9e89b51

- remove use of pointers - use int for incrementing numbers

dmitshur approved these changes Apr 26, 2018

View reviewed changes

gmlewis approved these changes Apr 27, 2018

View reviewed changes

gmlewis merged commit 29336db into google:master Apr 27, 2018

dmitshur reviewed Apr 27, 2018

View reviewed changes

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Support preview Team Discussions API (google#889)

edb1e32

Fixes google#849.

parkhyukjun89 mentioned this pull request Jan 28, 2019

Support preview Team Discussions API #849

Closed

		@@ -0,0 +1,7 @@
		package github

		// TeamsService provides access to the team related functions

Support preview Team Discussions API #889

Support preview Team Discussions API #889

Uh oh!

Conversation

parkhyukjun89 commented Apr 3, 2018

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkhyukjun89 commented Apr 15, 2018

Uh oh!

dmitshur commented Apr 15, 2018

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Apr 18, 2018

Uh oh!

parkhyukjun89 commented Apr 19, 2018

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkhyukjun89 Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

parkhyukjun89 Apr 23, 2018 •

edited

Loading

dmitshur Apr 25, 2018 •

edited

Loading

dmitshur left a comment •

edited

Loading