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:
|
gmlewis
left a comment
There was a problem hiding this comment.
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.
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.
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.
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
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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) { |
gmlewis
left a comment
There was a problem hiding this comment.
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.