Revert "Revert Do to take 2 parameters."

This reverts commit 6b55015.

I've described full rationale for doing so in my comment at
#526 (comment).
I'll paste it here for convenience.

Today, I experimented with updating some code I had that uses go-github
to the new API and see if that would lead to any insight. For a good
typical example, see https://github.com/shurcooL/notifications/pull/1/files.

That perspective led me to better understanding, and I now think that the
better thing to do is to revert commit 6b55015 and use the following
signature for Do after all:

	Do(ctx context.Context, req *http.Request, v interface{})

Here are 4 reasons that make up the rationale/motivation for doing that:

1. First reason. After I switched to the new context branch of go-github
locally and ran tests on my Go code that uses go-github, the compiler gave
me very clear and actionable errors such as:

	# github.com/shurcooL/notifications/githubapi
	githubapi/githubapi.go:44: not enough arguments in call to s.clNoCache.Activity.ListNotifications
		have (nil)
		want (context.Context, *github.NotificationListOptions)
	githubapi/githubapi.go:53: not enough arguments in call to s.clNoCache.Activity.ListRepositoryNotifications
		have (string, string, nil)
		want (context.Context, string, string, *github.NotificationListOptions)
	githubapi/githubapi.go:151: not enough arguments in call to s.cl.Activity.ListRepositoryNotifications
		have (string, string, nil)
		want (context.Context, string, string, *github.NotificationListOptions)
	githubapi/githubapi.go:179: not enough arguments in call to s.cl.Activity.MarkThreadRead
		have (string)
		want (context.Context, string)
	githubapi/githubapi.go:193: not enough arguments in call to s.cl.Activity.MarkRepositoryNotificationsRead
		have (string, string, time.Time)
		want (context.Context, string, string, time.Time)
	FAIL	github.com/shurcooL/notifications/githubapi [build failed]

It was very helpful and very easy to update my code to pass the
additional context parameter. I had a great time doing it.

However, I only later noticed that I forgot about the calls to Do method
that some of my code made. That code continued to compile without any
errors, and while it worked, the context wasn't propagated. Spotting
that was hard, and worst of all, it felt very inconsistent.

When every single method has an extra parameter, which makes the compiler
help you catch instances of code you need to update, why doesn't that
also apply to calls to Do method?

Even after I updated my calls to Do to pass context with
req.WithContext(ctx), it was harder to be confident I hadn't missed some
cases. For example, imagine a situation where you set the context on a
request earlier, and then call Do(req, ...). In the end, having an
explicit first parameter for passing context is a lot easier to see
that context is being propagated.

2. Second reason. I believe my rationale in commit 6b55015 is not valid.
It definitely shouldn't carry as much weight as I originally thought.

The situation I described where req is created in another function and
then passed in... That just doesn't seem like something that would happen
often. It seems that code that creates a NewRequest and then does Do is
more likely. E.g., something like https://github.com/shurcooL/notifications/blob/a03ac7eff1ecb7f92f0d91e32674137cc017286c/githubapi/githubapi.go#L248-L256.

3. Third reason. I originally noticed that between the two options of
passing context separately and explicitly, and passing it via request,
the latter was more in line with Go standard library.

Clearly, the NewRequest and Do methods are modeled after same ones in
net/http package:

https://godoc.org/net/http#NewRequest
https://godoc.org/net/http#Client.Do

However, that's not evidence that it's the best way to pass context to
Do. The net/http package API is frozen in Go1 and it couldn't have
changed.

A better place to look would be golang/go#11904, a proposal to make a
friendlier context-aware http.Do which was resolved by creating the
golang.org/x/net/context/ctxhttp package. Look at its Do method:

	// Do sends an HTTP request with the provided http.Client and returns
	// an HTTP response.
	//
	// If the client is nil, http.DefaultClient is used.
	//
	// The provided ctx must be non-nil. If it is canceled or times out,
	// ctx.Err() will be returned.
	func Do(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) {
		if client == nil {
			client = http.DefaultClient
		}
		resp, err := client.Do(req.WithContext(ctx))
		// If we got an error, and the context has been canceled,
		// the context's error is probably more useful.
		if err != nil {
			select {
			case <-ctx.Done():
				err = ctx.Err()
			default:
			}
		}
		return resp, err
	}

That was the outcome when the Go1 API freeze did not constrain the choice of Do's signature.

4. Fourth reason. There is a proposal golang/go#16742 that tracks the
creation of a tool to help with validating correct context propagation.
Such a tool is hinted at in context package documentation, but so far
does not exist.

Had it existed and if it were trivial to verify that context is
propagated and not accidentally dropped, this reason would not be included.

The fact is that it's much easier for users to validate correct
propagation of context if the signature of Do is changed to accept
ctx context.Context explicitly as first parameter. So, this is additional
evidence I believe we should go with what's friendlier to users of the
API. As motivated by first reason, I believe it's friendlier to break the
API of Do method than it is not to break it (somewhat counter-intuitively).

For these reasons, I think we should make the change of Do to:

	Do(ctx context.Context, req *http.Request, v interface{})

Which this revert does.

Author: dmitshur

github/activity.go

@@ -60,7 +60,7 @@ func (s *ActivityService) ListFeeds(ctx context.Context) (*Feeds, *Response, err
 	}
 
 	f := &Feeds{}
-	resp, err := s.client.Do(req.WithContext(ctx), f)
+	resp, err := s.client.Do(ctx, req, f)
 	if err != nil {
 		return nil, resp, err
 	}

github/activity_events.go

@@ -110,7 +110,7 @@ func (s *ActivityService) ListEvents(ctx context.Context, opt *ListOptions) ([]*
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -134,7 +134,7 @@ func (s *ActivityService) ListRepositoryEvents(ctx context.Context, owner, repo
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -158,7 +158,7 @@ func (s *ActivityService) ListIssueEventsForRepository(ctx context.Context, owne
 	}
 
 	var events []*IssueEvent
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -182,7 +182,7 @@ func (s *ActivityService) ListEventsForRepoNetwork(ctx context.Context, owner, r
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -206,7 +206,7 @@ func (s *ActivityService) ListEventsForOrganization(ctx context.Context, org str
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -236,7 +236,7 @@ func (s *ActivityService) ListEventsPerformedByUser(ctx context.Context, user st
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -266,7 +266,7 @@ func (s *ActivityService) ListEventsReceivedByUser(ctx context.Context, user str
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -291,7 +291,7 @@ func (s *ActivityService) ListUserEventsForOrganization(ctx context.Context, org
 	}
 
 	var events []*Event
-	resp, err := s.client.Do(req.WithContext(ctx), &events)
+	resp, err := s.client.Do(ctx, req, &events)
 	if err != nil {
 		return nil, resp, err
 	}

github/activity_notifications.go

@@ -63,7 +63,7 @@ func (s *ActivityService) ListNotifications(ctx context.Context, opt *Notificati
 	}
 
 	var notifications []*Notification
-	resp, err := s.client.Do(req.WithContext(ctx), &notifications)
+	resp, err := s.client.Do(ctx, req, &notifications)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -88,7 +88,7 @@ func (s *ActivityService) ListRepositoryNotifications(ctx context.Context, owner
 	}
 
 	var notifications []*Notification
-	resp, err := s.client.Do(req.WithContext(ctx), &notifications)
+	resp, err := s.client.Do(ctx, req, &notifications)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -112,7 +112,7 @@ func (s *ActivityService) MarkNotificationsRead(ctx context.Context, lastRead ti
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // MarkRepositoryNotificationsRead marks all notifications up to lastRead in
@@ -129,7 +129,7 @@ func (s *ActivityService) MarkRepositoryNotificationsRead(ctx context.Context, o
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // GetThread gets the specified notification thread.
@@ -144,7 +144,7 @@ func (s *ActivityService) GetThread(ctx context.Context, id string) (*Notificati
 	}
 
 	notification := new(Notification)
-	resp, err := s.client.Do(req.WithContext(ctx), notification)
+	resp, err := s.client.Do(ctx, req, notification)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -163,7 +163,7 @@ func (s *ActivityService) MarkThreadRead(ctx context.Context, id string) (*Respo
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // GetThreadSubscription checks to see if the authenticated user is subscribed
@@ -179,7 +179,7 @@ func (s *ActivityService) GetThreadSubscription(ctx context.Context, id string)
 	}
 
 	sub := new(Subscription)
-	resp, err := s.client.Do(req.WithContext(ctx), sub)
+	resp, err := s.client.Do(ctx, req, sub)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -200,7 +200,7 @@ func (s *ActivityService) SetThreadSubscription(ctx context.Context, id string,
 	}
 
 	sub := new(Subscription)
-	resp, err := s.client.Do(req.WithContext(ctx), sub)
+	resp, err := s.client.Do(ctx, req, sub)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -219,5 +219,5 @@ func (s *ActivityService) DeleteThreadSubscription(ctx context.Context, id strin
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }

github/activity_star.go

@@ -41,7 +41,7 @@ func (s *ActivityService) ListStargazers(ctx context.Context, owner, repo string
 	req.Header.Set("Accept", mediaTypeStarringPreview)
 
 	var stargazers []*Stargazer
-	resp, err := s.client.Do(req.WithContext(ctx), &stargazers)
+	resp, err := s.client.Do(ctx, req, &stargazers)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -88,7 +88,7 @@ func (s *ActivityService) ListStarred(ctx context.Context, user string, opt *Act
 	req.Header.Set("Accept", mediaTypeStarringPreview)
 
 	var repos []*StarredRepository
-	resp, err := s.client.Do(req.WithContext(ctx), &repos)
+	resp, err := s.client.Do(ctx, req, &repos)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -105,7 +105,7 @@ func (s *ActivityService) IsStarred(ctx context.Context, owner, repo string) (bo
 	if err != nil {
 		return false, nil, err
 	}
-	resp, err := s.client.Do(req.WithContext(ctx), nil)
+	resp, err := s.client.Do(ctx, req, nil)
 	starred, err := parseBoolResponse(err)
 	return starred, resp, err
 }
@@ -119,7 +119,7 @@ func (s *ActivityService) Star(ctx context.Context, owner, repo string) (*Respon
 	if err != nil {
 		return nil, err
 	}
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // Unstar a repository as the authenticated user.
@@ -131,5 +131,5 @@ func (s *ActivityService) Unstar(ctx context.Context, owner, repo string) (*Resp
 	if err != nil {
 		return nil, err
 	}
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }

github/activity_watching.go

@@ -41,7 +41,7 @@ func (s *ActivityService) ListWatchers(ctx context.Context, owner, repo string,
 	}
 
 	var watchers []*User
-	resp, err := s.client.Do(req.WithContext(ctx), &watchers)
+	resp, err := s.client.Do(ctx, req, &watchers)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -71,7 +71,7 @@ func (s *ActivityService) ListWatched(ctx context.Context, user string, opt *Lis
 	}
 
 	var watched []*Repository
-	resp, err := s.client.Do(req.WithContext(ctx), &watched)
+	resp, err := s.client.Do(ctx, req, &watched)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -93,7 +93,7 @@ func (s *ActivityService) GetRepositorySubscription(ctx context.Context, owner,
 	}
 
 	sub := new(Subscription)
-	resp, err := s.client.Do(req.WithContext(ctx), sub)
+	resp, err := s.client.Do(ctx, req, sub)
 	if err != nil {
 		// if it's just a 404, don't return that as an error
 		_, err = parseBoolResponse(err)
@@ -120,7 +120,7 @@ func (s *ActivityService) SetRepositorySubscription(ctx context.Context, owner,
 	}
 
 	sub := new(Subscription)
-	resp, err := s.client.Do(req.WithContext(ctx), sub)
+	resp, err := s.client.Do(ctx, req, sub)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -142,5 +142,5 @@ func (s *ActivityService) DeleteRepositorySubscription(ctx context.Context, owne
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }

github/admin.go

@@ -73,7 +73,7 @@ func (s *AdminService) UpdateUserLDAPMapping(ctx context.Context, user string, m
 	}
 
 	m := new(UserLDAPMapping)
-	resp, err := s.client.Do(req.WithContext(ctx), m)
+	resp, err := s.client.Do(ctx, req, m)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -92,7 +92,7 @@ func (s *AdminService) UpdateTeamLDAPMapping(ctx context.Context, team int, mapp
 	}
 
 	m := new(TeamLDAPMapping)
-	resp, err := s.client.Do(req.WithContext(ctx), m)
+	resp, err := s.client.Do(ctx, req, m)
 	if err != nil {
 		return nil, resp, err
 	}

github/authorizations.go

@@ -150,7 +150,7 @@ func (s *AuthorizationsService) List(ctx context.Context, opt *ListOptions) ([]*
 	}
 
 	var auths []*Authorization
-	resp, err := s.client.Do(req.WithContext(ctx), &auths)
+	resp, err := s.client.Do(ctx, req, &auths)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -169,7 +169,7 @@ func (s *AuthorizationsService) Get(ctx context.Context, id int) (*Authorization
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -188,7 +188,7 @@ func (s *AuthorizationsService) Create(ctx context.Context, auth *AuthorizationR
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -223,7 +223,7 @@ func (s *AuthorizationsService) GetOrCreateForApp(ctx context.Context, clientID
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -243,7 +243,7 @@ func (s *AuthorizationsService) Edit(ctx context.Context, id int, auth *Authoriz
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -262,7 +262,7 @@ func (s *AuthorizationsService) Delete(ctx context.Context, id int) (*Response,
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // Check if an OAuth token is valid for a specific app.
@@ -283,7 +283,7 @@ func (s *AuthorizationsService) Check(ctx context.Context, clientID string, toke
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -311,7 +311,7 @@ func (s *AuthorizationsService) Reset(ctx context.Context, clientID string, toke
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -334,7 +334,7 @@ func (s *AuthorizationsService) Revoke(ctx context.Context, clientID string, tok
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // ListGrants lists the set of OAuth applications that have been granted
@@ -350,7 +350,7 @@ func (s *AuthorizationsService) ListGrants(ctx context.Context) ([]*Grant, *Resp
 	}
 
 	grants := []*Grant{}
-	resp, err := s.client.Do(req.WithContext(ctx), &grants)
+	resp, err := s.client.Do(ctx, req, &grants)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -369,7 +369,7 @@ func (s *AuthorizationsService) GetGrant(ctx context.Context, id int) (*Grant, *
 	}
 
 	grant := new(Grant)
-	resp, err := s.client.Do(req.WithContext(ctx), grant)
+	resp, err := s.client.Do(ctx, req, grant)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -389,7 +389,7 @@ func (s *AuthorizationsService) DeleteGrant(ctx context.Context, id int) (*Respo
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }
 
 // CreateImpersonation creates an impersonation OAuth token.
@@ -407,7 +407,7 @@ func (s *AuthorizationsService) CreateImpersonation(ctx context.Context, usernam
 	}
 
 	a := new(Authorization)
-	resp, err := s.client.Do(req.WithContext(ctx), a)
+	resp, err := s.client.Do(ctx, req, a)
 	if err != nil {
 		return nil, resp, err
 	}
@@ -426,5 +426,5 @@ func (s *AuthorizationsService) DeleteImpersonation(ctx context.Context, usernam
 		return nil, err
 	}
 
-	return s.client.Do(req.WithContext(ctx), nil)
+	return s.client.Do(ctx, req, nil)
 }