Skip to content

Repository navigation

Update for go-github API change. #70

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

Merged
dmitshur merged 1 commit into from
Feb 20, 2017
Merged

Update for go-github API change. #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion presentation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package gps

import "context"

// Presentation provides infomation about a Go package repo with an available update.
type Presentation struct {
HomeURL string // Home URL of the Go package. Optional (empty string means none available).
Expand All @@ -22,4 +24,4 @@ type Comments struct {
}

// Presenter returns a Presentation for r, or nil if it can't.
type Presenter func(r *Repo) *Presentation
type Presenter func(ctx context.Context, r *Repo) *Presentation
17 changes: 9 additions & 8 deletions presenter/github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
package github

import (
"context"
"fmt"
"net/http"
"strings"
Expand All @@ -18,47 +19,47 @@ func NewPresenter(httpClient *http.Client) gps.Presenter {
gh := github.NewClient(httpClient)
gh.UserAgent = "github.com/shurcooL/Go-Package-Store/presenter/github"

return func(repo *gps.Repo) *gps.Presentation {
return func(ctx context.Context, repo *gps.Repo) *gps.Presentation {
switch {
// Import path begins with "github.com/".
case strings.HasPrefix(repo.Root, "github.com/"):
elems := strings.Split(repo.Root, "/")
if len(elems) != 3 {
return nil
}
return presentGitHubRepo(gh, repo, elems[1], elems[2])
return presentGitHubRepo(ctx, gh, repo, elems[1], elems[2])
// gopkg.in package.
case strings.HasPrefix(repo.Root, "gopkg.in/"):
githubOwner, githubRepo, err := gopkgInImportPathToGitHub(repo.Root)
if err != nil {
return nil
}
return presentGitHubRepo(gh, repo, githubOwner, githubRepo)
return presentGitHubRepo(ctx, gh, repo, githubOwner, githubRepo)
// Underlying GitHub remote.
case strings.HasPrefix(repo.Remote.RepoURL, "https://github.com/"):
elems := strings.Split(strings.TrimSuffix(repo.Remote.RepoURL[len("https://"):], ".git"), "/")
if len(elems) != 3 {
return nil
}
return presentGitHubRepo(gh, repo, elems[1], elems[2])
return presentGitHubRepo(ctx, gh, repo, elems[1], elems[2])
// Go repo remote has a GitHub mirror repo.
case strings.HasPrefix(repo.Remote.RepoURL, "https://go.googlesource.com/"):
repoName := repo.Remote.RepoURL[len("https://go.googlesource.com/"):]
return presentGitHubRepo(gh, repo, "golang", repoName)
return presentGitHubRepo(ctx, gh, repo, "golang", repoName)
default:
return nil
}
}
}

func presentGitHubRepo(gh *github.Client, repo *gps.Repo, ghOwner, ghRepo string) *gps.Presentation {
func presentGitHubRepo(ctx context.Context, gh *github.Client, repo *gps.Repo, ghOwner, ghRepo string) *gps.Presentation {
p := &gps.Presentation{
HomeURL: "https://" + repo.Root,
ImageURL: "https://github.com/images/gravatars/gravatar-user-420.png", // Default fallback.
}

// This might take a while.
if cc, _, err := gh.Repositories.CompareCommits(ghOwner, ghRepo, repo.Local.Revision, repo.Remote.Revision); err == nil {
if cc, _, err := gh.Repositories.CompareCommits(ctx, ghOwner, ghRepo, repo.Local.Revision, repo.Remote.Revision); err == nil {
p.Changes = extractChanges(cc)
} else if rateLimitErr, ok := err.(*github.RateLimitError); ok {
setFirstError(p, rateLimitError{rateLimitErr})
Expand All @@ -67,7 +68,7 @@ func presentGitHubRepo(gh *github.Client, repo *gps.Repo, ghOwner, ghRepo string
}

// Use the repo owner avatar image.
if user, _, err := gh.Users.Get(ghOwner); err == nil && user.AvatarURL != nil {
if user, _, err := gh.Users.Get(ctx, ghOwner); err == nil && user.AvatarURL != nil {
p.ImageURL = *user.AvatarURL
} else if rateLimitErr, ok := err.(*github.RateLimitError); ok {
setFirstError(p, rateLimitError{rateLimitErr})
Expand Down
13 changes: 7 additions & 6 deletions presenter/gitiles/gitiles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package gitiles

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
Expand All @@ -16,19 +17,19 @@ import (
// httpClient is the HTTP client to be used by the presenter for accessing the Gitiles API.
// If httpClient is nil, then http.DefaultClient is used.
func NewPresenter(httpClient *http.Client) gps.Presenter {
return func(repo *gps.Repo) *gps.Presentation {
return func(ctx context.Context, repo *gps.Repo) *gps.Presentation {
switch {
case strings.HasPrefix(repo.Remote.RepoURL, "https://code.googlesource.com/"):
return presentGitilesRepo(httpClient, repo)
return presentGitilesRepo(ctx, httpClient, repo)
default:
return nil
}
}
}

func presentGitilesRepo(client *http.Client, repo *gps.Repo) *gps.Presentation {
func presentGitilesRepo(ctx context.Context, client *http.Client, repo *gps.Repo) *gps.Presentation {
// This might take a while.
log, err := fetchLog(client, repo.Remote.RepoURL+"/+log?format=JSON")
log, err := fetchLog(ctx, client, repo.Remote.RepoURL+"/+log?format=JSON")
if err != nil {
return &gps.Presentation{Error: err}
}
Expand All @@ -41,13 +42,13 @@ func presentGitilesRepo(client *http.Client, repo *gps.Repo) *gps.Presentation {
}

// fetchLog fetches a Gitiles log at a given url, using client.
func fetchLog(client *http.Client, url string) (log, error) {
func fetchLog(ctx context.Context, client *http.Client, url string) (log, error) {
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return log{}, err
}
req.Header.Set("User-Agent", "github.com/shurcooL/Go-Package-Store/presenter/gitiles")
resp, err := client.Do(req)
resp, err := client.Do(req.WithContext(ctx))
if err != nil {
return log{}, err
}
Expand Down
3 changes: 2 additions & 1 deletion presenter/gitiles/gitiles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package gitiles

import (
"context"
"net/http"
"os"
"path/filepath"
Expand All @@ -22,7 +23,7 @@ func TestFetchLog(t *testing.T) {
}),
}

log, err := fetchLog(client, "")
log, err := fetchLog(context.Background(), client, "")

@emidoots emidoots Feb 19, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be context.TODO() since it's not really something intended to run in the background? I ask because I really don't know 😄

@dmitshur dmitshur Feb 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment @slimsag!

context.TODO() exists so you can write code like this:

fetchLog(nil /* TODO: Figure out what context I want to use here. */, client, "")

But without actually using nil, because you should never pass nil context.Context value. See its documentation:

TODO returns a non-nil, empty Context. Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter). TODO is recognized by static analysis tools that determine whether Contexts are propagated correctly in a program.

And context.Background() doesn't mean mean it's something that should run in background, as far as I know. Its documentation says:

Background returns a non-nil, empty Context. It is never canceled, has no values, and has no deadline. It is typically used by the main function, initialization, and tests, and as the top-level Context for incoming requests.

Which is exactly what I want.

Basically, context.TODO() means the code is unfinished (and you're not sure which context you're going to use). context.Background() is just a normal top-level empty context.

Also see google/go-github@ed4cdce which is relevant.

if err != nil {
t.Fatalf("fetchLog: %v", err)
}
Expand Down
3 changes: 2 additions & 1 deletion workspace/workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
package workspace

import (
"context"
"fmt"
"go/build"
"log"
Expand Down Expand Up @@ -635,7 +636,7 @@ func (p *Pipeline) presentWorker(wg *sync.WaitGroup) {
// but falls back to a generic presentation if there's nothing better.
func (p *Pipeline) present(repo *gps.Repo) *gps.Presentation {
for _, presenter := range p.presenters {
if presentation := presenter(repo); presentation != nil {
if presentation := presenter(context.Background(), repo); presentation != nil {
return presentation
}
}
Expand Down