Support Preview hovercard API (#875) #890

monicbhanushali · 2018-04-08T15:37:38Z

Add support for new preview Hovercard API as described in:
https://developer.github.com/changes/2018-03-21-hovercard-api-preview/

This PR adds:

A new method to support the new Hovercard API.

Fixes: #875

…port-hovercard-api

googlebot · 2018-04-08T15:37:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

monicbhanushali · 2018-04-08T15:43:38Z

I signed it!

googlebot · 2018-04-08T15:43:41Z

CLAs look good, thanks!

dmitshur

Thanks for working on this. It's a good start.

I see some opportunities to improve this PR. I've left initial comments.

dmitshur · 2018-04-08T16:57:46Z

github/users.go

+}
+
+// UserContextualInfoResponse holds the parsed response from GetContextualInfo.
+type UserContextualInfoResponse struct {


To be more consistent with the existing API, let's rename this to UserContextualInfo (without Response suffix).

Actually, what do you think about calling it Hovercard? It's what the API endpoint is called:

GET /users/:username/hovercard

This came to my mind but then I thought to keep the names consistent and hence changed it to UserContextualInfoResponse, but Hovercard seems much better name considering the API endpoint.

dmitshur · 2018-04-08T16:58:40Z

github/users.go

+// UserContext represents the contextual information about user.
+type UserContext struct {
+	Message *string `url:"message,omitempty"`
+	Octicon *string `url:"octicon,omitempty"`


Why is it url in the struct field tag? Shouldn't it be json?

dmitshur · 2018-04-08T16:58:49Z

github/users.go

+// method.
+type UserContextualInfoOptions struct {
+	// SubjectType specifies the additional information to be received about the hovercard.
+	// Possible values are : organization, repository, issue, pull_request.


Remove space before colon.

dmitshur · 2018-04-08T16:59:53Z

github/users.go

+		return nil, nil, err
+	}
+
+	//TODO : remove custom Accept header when this API fully launches.


The spacing around TODO is a little unusual. Let's make it consistent. Move the space before colon to before TODO:

// TODO: remove custom Accept header when this API fully launches.

dmitshur · 2018-04-08T17:00:18Z

github/users.go

+	//TODO : remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeHovercardPreview)
+
+	contextResp := new(UserContextualInfoResponse)


Give this variable a shorter name. Like uci (for User Contextual Info).

dmitshur · 2018-04-08T17:07:24Z

github/users.go

@@ -134,6 +134,55 @@ func (s *UsersService) Edit(ctx context.Context, user *User) (*User, *Response,
 	return uResp, resp, nil
 }

+// UserContextualInfoOptions specifies optional parameters to the UsersService.GetContextualInfo
+// method.
+type UserContextualInfoOptions struct {


This will be HovercardOptions then.

dmitshur · 2018-04-08T17:08:39Z

github/users.go

+	SubjectType string `url:"subject_type,omitempty"`
+
+	// SubjectID specifies the ID for the SubjectType.
+	SubjectID string `url:"subject_id,omitempty"`


Both the parameters are required, not optional. So remove ,omitempty and add (Required.) at the end of comment (like here).

monicbhanushali · 2018-04-09T16:58:16Z

Thanks for the suggestions @shurcooL .
PR has been updated to address the suggested changes.

monicbhanushali · 2018-04-12T06:29:11Z

To use Hovercard api, authentication needs to be in place.
Should a comment describing this be added in the new method?

dmitshur · 2018-04-12T21:21:38Z

Should a comment describing this be added in the new method?

Good call, let's do that.

GitHub API v3 already documents that requirement at https://developer.github.com/v3/users/#get-contextual-information-about-a-user, but it will be nice if we mention it here too.

UsersService.GetGPGKey method serves as a good template for how to do it:

go-github/github/users_gpg_keys.go

Lines 77 to 81 in 89a446e

    
           // GetGPGKey gets extended details for a single GPG key. It requires authentication 
        
           // via Basic Auth or via OAuth with at least read:gpg_key scope. 
        
           // 
        
           // GitHub API docs: https://developer.github.com/v3/users/gpg_keys/#get-a-single-gpg-key 
        
           func (s *UsersService) GetGPGKey(ctx context.Context, id int64) (*GPGKey, *Response, error) {

Also, given we already renamed UserContextualInfoResponse to Hovercard and the endpoint is GET /users/:username/hovercard, I think we should apply the same change to the method name. It can be renamed to GetHovercard. What do you think?

So, it can look something like this:

// GetHovercard gets hovercard information for specified user. It requires authentication
// via Basic Auth or via OAuth with the repo scope.
//
// GitHub API docs: https://developer.github.com/v3/users/#get-contextual-information-about-a-user
func (s *UsersService) GetHovercard(ctx context.Context, user string, opt *HovercardOptions) (*Hovercard, *Response, error)

Note that you should return the entire *Hovercard from that method, not just []*UserContext. That way, if the hovercard gets extra fields in the future, they'll be a part of the response.

dmitshur · 2018-04-12T21:23:01Z

github/users.go

+// GetContextualInfo fetches contextual information about user.
+//
+// GitHub API docs: https://developer.github.com/v3/users/#get-contextual-information-about-a-user
+func (s *UsersService) GetContextualInfo(ctx context.Context, user string, opt *HovercardOptions) ([]*UserContext, *Response, error) {


Both options in HovercardOptions are required. So, we should make opt mandatory here as well. No need to make it a pointer, since nil value can never be valid.

I revisited the API documentation and it says "The subject_type and subject_id parameters provide context for the person's hovercard, which returns more information than without the parameters".
I tried using the API service without parameters and the response is empty contexts array. (HTTP response code is 200)

Response without parameters:
{ "contexts": [ ] }

So I was thinking whether to keep these parameters optional or mandatory ? Would like to hear your views @shurcooL

Hey, any views on this?

Sorry about the delay.

I see, it looks like they've updated the documentation to say that the two parameters are not mandatory, but if one is used, then the other is required too:

That means passing neither parameter is a valid operation.

So, we should keep it a pointer then, so it'd be possible to pass nil value for *opt HovercardOptions when both parameters are being left out.

We should also update the documentation of HovercardOptions fields. Instead of "(Required.)", it should say something like "(Required when using subject_id.)" and "(Required when using subject_type.)", respectively.

Great! Will keep the opt optional and update the documentation. Should be done by today. Thanks @shurcooL

dmitshur · 2018-04-12T21:29:38Z

github/users.go

 	req.Header.Set("Accept", mediaTypeHovercardPreview)

-	contextResp := new(UserContextualInfoResponse)
-	resp, err := s.client.Do(ctx, req, contextResp)
+	uci := new(Hovercard)


uci is no longer applicable. Make it hc or hovercard.

Oops Sorry! missed it.
I will make the suggested improvements.

monicbhanushali · 2018-05-08T16:41:14Z

I have made the suggested changes.
Apologies for taking so long.

monicbhanushali · 2018-05-22T12:55:01Z

Hey @shurcooL , any views on these changes ? Let me know if it looks good to you

dmitshur

Thanks for working on this @MonicB14. The latest changes look good and this has my LGTM now. There's just one minor godoc issue (missing blank line in a comment) and a few optional suggestions. See inline comments.

dmitshur · 2018-05-22T17:16:52Z

github/users.go

+	Octicon *string `json:"octicon,omitempty"`
+}
+
+// Hovercard holds the parsed response from GetHovercard.


It'd be nice if we could better describe what this struct is, rather than saying it's "what GetHovercard returns". Perhaps something like this?

// Hovercard represents hovercard information about a user.

It's not great, so you can go either way here.

dmitshur · 2018-05-22T17:16:55Z

github/users.go

+
+// GetHovercard fetches contextual information about user. It requires authentication
+// via Basic Auth or via OAuth with the repo scope.
+// GitHub API docs: https://developer.github.com/v3/users/#get-contextual-information-about-a-user


The // GitHub API docs: ... line needs an empty line above, otherwise it gets parsed by godoc as part of the same paragraph, making it less readable. See here for example:

go-github/github/users.go

Lines 118 to 120 in d4c2343

// Edit the authenticated user.

//

// GitHub API docs: https://developer.github.com/v3/users/#update-the-authenticated-user

dmitshur · 2018-05-22T17:18:59Z

github/users.go

+// Hovercard holds the parsed response from GetHovercard.
+type Hovercard struct {
+	Contexts []*UserContext `json:"contexts,omitempty"`
+}


It would be good to place Hovercard on top of UserContext. The general pattern is higher level identifiers should come first. They are easier to understand, and they set context for the lower level identifiers that follow. I.e.:

// Hovercard ... type Hovercard struct { ... } // UserContext ... type UserContext struct { ... }

googlebot · 2018-08-10T16:57:02Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Fixes #875. Closes #890.

Fixes google#875. Closes google#890.

monicbhanushali added 2 commits April 8, 2018 20:43

Support preview Hovercard API (google#875)

e2c6268

Merge branch 'master' of https://github.com/google/go-github into sup…

71d461d

…port-hovercard-api

googlebot added the cla: no label Apr 8, 2018

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Apr 8, 2018

dmitshur reviewed Apr 8, 2018

View reviewed changes

Addressed comments in code review.

c14be0c

dmitshur reviewed Apr 12, 2018

View reviewed changes

Implementing code review suggestions.

d4c2343

dmitshur approved these changes May 22, 2018

View reviewed changes

Address review feedback from @dmitshur in google#890

2eda43b

googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Aug 10, 2018

Merge branch 'master' into support-hovercard-api

ffb9aa9

gmlewis mentioned this pull request Aug 10, 2018

Support Preview Hovercard API #973

Merged

gmlewis closed this in #973 Aug 10, 2018

gmlewis added a commit that referenced this pull request Aug 10, 2018

Support Preview Hovercard API (#973)

5d3c7cb

Fixes #875. Closes #890.

gmlewis added a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018

Support Preview Hovercard API (google#973)

5cfbf19

Fixes google#875. Closes google#890.

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Support Preview Hovercard API (google#973)

a5b3b89

Fixes google#875. Closes google#890.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Preview hovercard API (#875) #890

Support Preview hovercard API (#875) #890

monicbhanushali commented Apr 8, 2018

googlebot commented Apr 8, 2018

monicbhanushali commented Apr 8, 2018

googlebot commented Apr 8, 2018

dmitshur left a comment

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

monicbhanushali Apr 9, 2018

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

dmitshur Apr 8, 2018

monicbhanushali commented Apr 9, 2018

monicbhanushali commented Apr 12, 2018

dmitshur commented Apr 12, 2018

dmitshur Apr 12, 2018

monicbhanushali Apr 22, 2018 •

edited

Loading

monicbhanushali Apr 25, 2018

dmitshur Apr 25, 2018 •

edited

Loading

monicbhanushali Apr 27, 2018

dmitshur Apr 12, 2018

monicbhanushali Apr 13, 2018

monicbhanushali commented May 8, 2018

monicbhanushali commented May 22, 2018 •

edited

Loading

dmitshur left a comment

dmitshur May 22, 2018

dmitshur May 22, 2018

dmitshur May 22, 2018

googlebot commented Aug 10, 2018

	// Edit the authenticated user.
	//
	// GitHub API docs: https://developer.github.com/v3/users/#update-the-authenticated-user

Support Preview hovercard API (#875) #890

Support Preview hovercard API (#875) #890

Conversation

monicbhanushali commented Apr 8, 2018

googlebot commented Apr 8, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

monicbhanushali commented Apr 8, 2018

googlebot commented Apr 8, 2018

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monicbhanushali commented Apr 9, 2018

monicbhanushali commented Apr 12, 2018

dmitshur commented Apr 12, 2018

Choose a reason for hiding this comment

monicbhanushali Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monicbhanushali commented May 8, 2018

monicbhanushali commented May 22, 2018 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Aug 10, 2018

monicbhanushali Apr 22, 2018 •

edited

Loading

dmitshur Apr 25, 2018 •

edited

Loading

monicbhanushali commented May 22, 2018 •

edited

Loading