- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.2k
Add Social Accounts API #3647
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.
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
Add Social Accounts API #3647
Conversation
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##           master    #3647      +/-   ##
==========================================
- Coverage   91.25%   91.24%   -0.01%     
==========================================
  Files         184      185       +1     
  Lines       16311    16362      +51     
==========================================
+ Hits        14884    14929      +45     
- Misses       1245     1249       +4     
- Partials      182      184       +2     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Pedja-Djape.
I realize this is in Draft mode, but wanted to give you some guidance before you finish the PR by writing unit tests.
…]string instead of []*strin
| Please switch this from "Draft" to "Ready for review" when you are ready, @Pedja-Djape. Thank you! | 
| 
 Done! Thank you for all the help :) | 
        
          
                example/social_accounts/main.go
              
                Outdated
          
        
      | accounts, err := fetchSocialAccounts(username) | ||
| if err != nil { | ||
| log.Fatalf("Error: %v\n", err) | ||
| return | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete this line. The nice thing about log.Fatalf is that it immediately kills the program, so the return is not necessary.
Also, please be sure to run gofmt on this file after you edit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't know about that log.Fatalf behavior, nice. Thanks for the heads up
        
          
                example/social_accounts/main.go
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving this example to examples_test.go, since it looks quite straightforward:
func ExampleUsersService_ListUserSocialAccounts() {
	client := github.NewClient(nil)
	users, _, err := client.Users.ListUserSocialAccounts(context.Background(), username, &github.ListOptions{})
	if err != nil {
		log.Fatalf("Failed to list user social accounts: %v", err)
	}
	// ...
}See
go-github/github/examples_test.go
Lines 96 to 112 in a8401a5
| func ExampleUsersService_ListAll() { | |
| client := github.NewClient(nil) | |
| opts := &github.UserListOptions{} | |
| for { | |
| ctx := context.Background() | |
| users, _, err := client.Users.ListAll(ctx, opts) | |
| if err != nil { | |
| log.Fatalf("error listing users: %v", err) | |
| } | |
| if len(users) == 0 { | |
| break | |
| } | |
| opts.Since = *users[len(users)-1].ID | |
| // Process users... | |
| } | |
| } | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi  @alexandear thanks for the review. Just want to clarify something before making changes: Are you saying that I should add the test in the examples_test.go file and remove this file? Or add the test in examples_test.go file and keep this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add the test in the examples_test.go file and remove this file
This is what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks!
        
          
                github/users_social_accounts.go
              
                Outdated
          
        
      | // GitHub API docs: https://docs.github.com/rest/users/social-accounts#add-social-accounts-for-the-authenticated-user | ||
| // | ||
| //meta:operation POST /user/social_accounts | ||
| func (s *UsersService) AddSocialAccounts(ctx context.Context, accountURLsToAdd []string) ([]*SocialAccount, *Response, error) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify naming:
| func (s *UsersService) AddSocialAccounts(ctx context.Context, accountURLsToAdd []string) ([]*SocialAccount, *Response, error) { | |
| func (s *UsersService) AddSocialAccounts(ctx context.Context, accountURLs []string) ([]*SocialAccount, *Response, error) { | 
        
          
                github/users_social_accounts.go
              
                Outdated
          
        
      | return nil, nil, err | ||
| } | ||
|  | ||
| var addedAccounts []*SocialAccount | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify:
| var addedAccounts []*SocialAccount | |
| var accounts []*SocialAccount | 
        
          
                github/users_social_accounts.go
              
                Outdated
          
        
      | // GitHub API docs: https://docs.github.com/rest/users/social-accounts#delete-social-accounts-for-the-authenticated-user | ||
| // | ||
| //meta:operation DELETE /user/social_accounts | ||
| func (s *UsersService) DeleteSocialAccounts(ctx context.Context, accountsToDelete []string) (*Response, error) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with CreateSocialAccounts:
| func (s *UsersService) DeleteSocialAccounts(ctx context.Context, accountsToDelete []string) (*Response, error) { | |
| func (s *UsersService) DeleteSocialAccounts(ctx context.Context, accountURLs []string) (*Response, error) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Pedja-Djape!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
| Whups, sorry, it looks like there are some linter errors that need addressing first. Please take a look at these and push the necessary changes, @Pedja-Djape. | 
| 
 Hey @gmlewis it seems like the lint job isn't failing because of lint errors but rather due to a  EDIT: NVM! | 
| 
 Hey @gmlewis I noticed the lint job isn't failing becuse of lint issues but rather due to a 
 Actually @gmlewis I confirmed my  | 
| Thank you, @Pedja-Djape and @alexandear! | 
This pull request aims to implement the API for social accounts mentioned in Issue-3634
Fixes: #3634.