Skip to content

Commit 74bd2e5

Browse files
dmitshurdmitshur
committed
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.
1 parent 9d36c75 commit 74bd2e5

64 files changed

Lines changed: 357 additions & 357 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎github/activity.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func (s *ActivityService) ListFeeds(ctx context.Context) (*Feeds, *Response, err
6060
}
6161

6262
f := &Feeds{}
63-
resp, err := s.client.Do(req.WithContext(ctx), f)
63+
resp, err := s.client.Do(ctx, req, f)
6464
if err != nil {
6565
return nil, resp, err
6666
}

‎github/activity_events.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func (s *ActivityService) ListEvents(ctx context.Context, opt *ListOptions) ([]*
110110
}
111111

112112
var events []*Event
113-
resp, err := s.client.Do(req.WithContext(ctx), &events)
113+
resp, err := s.client.Do(ctx, req, &events)
114114
if err != nil {
115115
return nil, resp, err
116116
}
@@ -134,7 +134,7 @@ func (s *ActivityService) ListRepositoryEvents(ctx context.Context, owner, repo
134134
}
135135

136136
var events []*Event
137-
resp, err := s.client.Do(req.WithContext(ctx), &events)
137+
resp, err := s.client.Do(ctx, req, &events)
138138
if err != nil {
139139
return nil, resp, err
140140
}
@@ -158,7 +158,7 @@ func (s *ActivityService) ListIssueEventsForRepository(ctx context.Context, owne
158158
}
159159

160160
var events []*IssueEvent
161-
resp, err := s.client.Do(req.WithContext(ctx), &events)
161+
resp, err := s.client.Do(ctx, req, &events)
162162
if err != nil {
163163
return nil, resp, err
164164
}
@@ -182,7 +182,7 @@ func (s *ActivityService) ListEventsForRepoNetwork(ctx context.Context, owner, r
182182
}
183183

184184
var events []*Event
185-
resp, err := s.client.Do(req.WithContext(ctx), &events)
185+
resp, err := s.client.Do(ctx, req, &events)
186186
if err != nil {
187187
return nil, resp, err
188188
}
@@ -206,7 +206,7 @@ func (s *ActivityService) ListEventsForOrganization(ctx context.Context, org str
206206
}
207207

208208
var events []*Event
209-
resp, err := s.client.Do(req.WithContext(ctx), &events)
209+
resp, err := s.client.Do(ctx, req, &events)
210210
if err != nil {
211211
return nil, resp, err
212212
}
@@ -236,7 +236,7 @@ func (s *ActivityService) ListEventsPerformedByUser(ctx context.Context, user st
236236
}
237237

238238
var events []*Event
239-
resp, err := s.client.Do(req.WithContext(ctx), &events)
239+
resp, err := s.client.Do(ctx, req, &events)
240240
if err != nil {
241241
return nil, resp, err
242242
}
@@ -266,7 +266,7 @@ func (s *ActivityService) ListEventsReceivedByUser(ctx context.Context, user str
266266
}
267267

268268
var events []*Event
269-
resp, err := s.client.Do(req.WithContext(ctx), &events)
269+
resp, err := s.client.Do(ctx, req, &events)
270270
if err != nil {
271271
return nil, resp, err
272272
}
@@ -291,7 +291,7 @@ func (s *ActivityService) ListUserEventsForOrganization(ctx context.Context, org
291291
}
292292

293293
var events []*Event
294-
resp, err := s.client.Do(req.WithContext(ctx), &events)
294+
resp, err := s.client.Do(ctx, req, &events)
295295
if err != nil {
296296
return nil, resp, err
297297
}

‎github/activity_notifications.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func (s *ActivityService) ListNotifications(ctx context.Context, opt *Notificati
6363
}
6464

6565
var notifications []*Notification
66-
resp, err := s.client.Do(req.WithContext(ctx), &notifications)
66+
resp, err := s.client.Do(ctx, req, &notifications)
6767
if err != nil {
6868
return nil, resp, err
6969
}
@@ -88,7 +88,7 @@ func (s *ActivityService) ListRepositoryNotifications(ctx context.Context, owner
8888
}
8989

9090
var notifications []*Notification
91-
resp, err := s.client.Do(req.WithContext(ctx), &notifications)
91+
resp, err := s.client.Do(ctx, req, &notifications)
9292
if err != nil {
9393
return nil, resp, err
9494
}
@@ -112,7 +112,7 @@ func (s *ActivityService) MarkNotificationsRead(ctx context.Context, lastRead ti
112112
return nil, err
113113
}
114114

115-
return s.client.Do(req.WithContext(ctx), nil)
115+
return s.client.Do(ctx, req, nil)
116116
}
117117

118118
// MarkRepositoryNotificationsRead marks all notifications up to lastRead in
@@ -129,7 +129,7 @@ func (s *ActivityService) MarkRepositoryNotificationsRead(ctx context.Context, o
129129
return nil, err
130130
}
131131

132-
return s.client.Do(req.WithContext(ctx), nil)
132+
return s.client.Do(ctx, req, nil)
133133
}
134134

135135
// GetThread gets the specified notification thread.
@@ -144,7 +144,7 @@ func (s *ActivityService) GetThread(ctx context.Context, id string) (*Notificati
144144
}
145145

146146
notification := new(Notification)
147-
resp, err := s.client.Do(req.WithContext(ctx), notification)
147+
resp, err := s.client.Do(ctx, req, notification)
148148
if err != nil {
149149
return nil, resp, err
150150
}
@@ -163,7 +163,7 @@ func (s *ActivityService) MarkThreadRead(ctx context.Context, id string) (*Respo
163163
return nil, err
164164
}
165165

166-
return s.client.Do(req.WithContext(ctx), nil)
166+
return s.client.Do(ctx, req, nil)
167167
}
168168

169169
// GetThreadSubscription checks to see if the authenticated user is subscribed
@@ -179,7 +179,7 @@ func (s *ActivityService) GetThreadSubscription(ctx context.Context, id string)
179179
}
180180

181181
sub := new(Subscription)
182-
resp, err := s.client.Do(req.WithContext(ctx), sub)
182+
resp, err := s.client.Do(ctx, req, sub)
183183
if err != nil {
184184
return nil, resp, err
185185
}
@@ -200,7 +200,7 @@ func (s *ActivityService) SetThreadSubscription(ctx context.Context, id string,
200200
}
201201

202202
sub := new(Subscription)
203-
resp, err := s.client.Do(req.WithContext(ctx), sub)
203+
resp, err := s.client.Do(ctx, req, sub)
204204
if err != nil {
205205
return nil, resp, err
206206
}
@@ -219,5 +219,5 @@ func (s *ActivityService) DeleteThreadSubscription(ctx context.Context, id strin
219219
return nil, err
220220
}
221221

222-
return s.client.Do(req.WithContext(ctx), nil)
222+
return s.client.Do(ctx, req, nil)
223223
}

‎github/activity_star.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (s *ActivityService) ListStargazers(ctx context.Context, owner, repo string
4141
req.Header.Set("Accept", mediaTypeStarringPreview)
4242

4343
var stargazers []*Stargazer
44-
resp, err := s.client.Do(req.WithContext(ctx), &stargazers)
44+
resp, err := s.client.Do(ctx, req, &stargazers)
4545
if err != nil {
4646
return nil, resp, err
4747
}
@@ -88,7 +88,7 @@ func (s *ActivityService) ListStarred(ctx context.Context, user string, opt *Act
8888
req.Header.Set("Accept", mediaTypeStarringPreview)
8989

9090
var repos []*StarredRepository
91-
resp, err := s.client.Do(req.WithContext(ctx), &repos)
91+
resp, err := s.client.Do(ctx, req, &repos)
9292
if err != nil {
9393
return nil, resp, err
9494
}
@@ -105,7 +105,7 @@ func (s *ActivityService) IsStarred(ctx context.Context, owner, repo string) (bo
105105
if err != nil {
106106
return false, nil, err
107107
}
108-
resp, err := s.client.Do(req.WithContext(ctx), nil)
108+
resp, err := s.client.Do(ctx, req, nil)
109109
starred, err := parseBoolResponse(err)
110110
return starred, resp, err
111111
}
@@ -119,7 +119,7 @@ func (s *ActivityService) Star(ctx context.Context, owner, repo string) (*Respon
119119
if err != nil {
120120
return nil, err
121121
}
122-
return s.client.Do(req.WithContext(ctx), nil)
122+
return s.client.Do(ctx, req, nil)
123123
}
124124

125125
// Unstar a repository as the authenticated user.
@@ -131,5 +131,5 @@ func (s *ActivityService) Unstar(ctx context.Context, owner, repo string) (*Resp
131131
if err != nil {
132132
return nil, err
133133
}
134-
return s.client.Do(req.WithContext(ctx), nil)
134+
return s.client.Do(ctx, req, nil)
135135
}

‎github/activity_watching.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (s *ActivityService) ListWatchers(ctx context.Context, owner, repo string,
4141
}
4242

4343
var watchers []*User
44-
resp, err := s.client.Do(req.WithContext(ctx), &watchers)
44+
resp, err := s.client.Do(ctx, req, &watchers)
4545
if err != nil {
4646
return nil, resp, err
4747
}
@@ -71,7 +71,7 @@ func (s *ActivityService) ListWatched(ctx context.Context, user string, opt *Lis
7171
}
7272

7373
var watched []*Repository
74-
resp, err := s.client.Do(req.WithContext(ctx), &watched)
74+
resp, err := s.client.Do(ctx, req, &watched)
7575
if err != nil {
7676
return nil, resp, err
7777
}
@@ -93,7 +93,7 @@ func (s *ActivityService) GetRepositorySubscription(ctx context.Context, owner,
9393
}
9494

9595
sub := new(Subscription)
96-
resp, err := s.client.Do(req.WithContext(ctx), sub)
96+
resp, err := s.client.Do(ctx, req, sub)
9797
if err != nil {
9898
// if it's just a 404, don't return that as an error
9999
_, err = parseBoolResponse(err)
@@ -120,7 +120,7 @@ func (s *ActivityService) SetRepositorySubscription(ctx context.Context, owner,
120120
}
121121

122122
sub := new(Subscription)
123-
resp, err := s.client.Do(req.WithContext(ctx), sub)
123+
resp, err := s.client.Do(ctx, req, sub)
124124
if err != nil {
125125
return nil, resp, err
126126
}
@@ -142,5 +142,5 @@ func (s *ActivityService) DeleteRepositorySubscription(ctx context.Context, owne
142142
return nil, err
143143
}
144144

145-
return s.client.Do(req.WithContext(ctx), nil)
145+
return s.client.Do(ctx, req, nil)
146146
}

‎github/admin.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func (s *AdminService) UpdateUserLDAPMapping(ctx context.Context, user string, m
7373
}
7474

7575
m := new(UserLDAPMapping)
76-
resp, err := s.client.Do(req.WithContext(ctx), m)
76+
resp, err := s.client.Do(ctx, req, m)
7777
if err != nil {
7878
return nil, resp, err
7979
}
@@ -92,7 +92,7 @@ func (s *AdminService) UpdateTeamLDAPMapping(ctx context.Context, team int, mapp
9292
}
9393

9494
m := new(TeamLDAPMapping)
95-
resp, err := s.client.Do(req.WithContext(ctx), m)
95+
resp, err := s.client.Do(ctx, req, m)
9696
if err != nil {
9797
return nil, resp, err
9898
}

‎github/authorizations.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func (s *AuthorizationsService) List(ctx context.Context, opt *ListOptions) ([]*
150150
}
151151

152152
var auths []*Authorization
153-
resp, err := s.client.Do(req.WithContext(ctx), &auths)
153+
resp, err := s.client.Do(ctx, req, &auths)
154154
if err != nil {
155155
return nil, resp, err
156156
}
@@ -169,7 +169,7 @@ func (s *AuthorizationsService) Get(ctx context.Context, id int) (*Authorization
169169
}
170170

171171
a := new(Authorization)
172-
resp, err := s.client.Do(req.WithContext(ctx), a)
172+
resp, err := s.client.Do(ctx, req, a)
173173
if err != nil {
174174
return nil, resp, err
175175
}
@@ -188,7 +188,7 @@ func (s *AuthorizationsService) Create(ctx context.Context, auth *AuthorizationR
188188
}
189189

190190
a := new(Authorization)
191-
resp, err := s.client.Do(req.WithContext(ctx), a)
191+
resp, err := s.client.Do(ctx, req, a)
192192
if err != nil {
193193
return nil, resp, err
194194
}
@@ -223,7 +223,7 @@ func (s *AuthorizationsService) GetOrCreateForApp(ctx context.Context, clientID
223223
}
224224

225225
a := new(Authorization)
226-
resp, err := s.client.Do(req.WithContext(ctx), a)
226+
resp, err := s.client.Do(ctx, req, a)
227227
if err != nil {
228228
return nil, resp, err
229229
}
@@ -243,7 +243,7 @@ func (s *AuthorizationsService) Edit(ctx context.Context, id int, auth *Authoriz
243243
}
244244

245245
a := new(Authorization)
246-
resp, err := s.client.Do(req.WithContext(ctx), a)
246+
resp, err := s.client.Do(ctx, req, a)
247247
if err != nil {
248248
return nil, resp, err
249249
}
@@ -262,7 +262,7 @@ func (s *AuthorizationsService) Delete(ctx context.Context, id int) (*Response,
262262
return nil, err
263263
}
264264

265-
return s.client.Do(req.WithContext(ctx), nil)
265+
return s.client.Do(ctx, req, nil)
266266
}
267267

268268
// 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
283283
}
284284

285285
a := new(Authorization)
286-
resp, err := s.client.Do(req.WithContext(ctx), a)
286+
resp, err := s.client.Do(ctx, req, a)
287287
if err != nil {
288288
return nil, resp, err
289289
}
@@ -311,7 +311,7 @@ func (s *AuthorizationsService) Reset(ctx context.Context, clientID string, toke
311311
}
312312

313313
a := new(Authorization)
314-
resp, err := s.client.Do(req.WithContext(ctx), a)
314+
resp, err := s.client.Do(ctx, req, a)
315315
if err != nil {
316316
return nil, resp, err
317317
}
@@ -334,7 +334,7 @@ func (s *AuthorizationsService) Revoke(ctx context.Context, clientID string, tok
334334
return nil, err
335335
}
336336

337-
return s.client.Do(req.WithContext(ctx), nil)
337+
return s.client.Do(ctx, req, nil)
338338
}
339339

340340
// ListGrants lists the set of OAuth applications that have been granted
@@ -350,7 +350,7 @@ func (s *AuthorizationsService) ListGrants(ctx context.Context) ([]*Grant, *Resp
350350
}
351351

352352
grants := []*Grant{}
353-
resp, err := s.client.Do(req.WithContext(ctx), &grants)
353+
resp, err := s.client.Do(ctx, req, &grants)
354354
if err != nil {
355355
return nil, resp, err
356356
}
@@ -369,7 +369,7 @@ func (s *AuthorizationsService) GetGrant(ctx context.Context, id int) (*Grant, *
369369
}
370370

371371
grant := new(Grant)
372-
resp, err := s.client.Do(req.WithContext(ctx), grant)
372+
resp, err := s.client.Do(ctx, req, grant)
373373
if err != nil {
374374
return nil, resp, err
375375
}
@@ -389,7 +389,7 @@ func (s *AuthorizationsService) DeleteGrant(ctx context.Context, id int) (*Respo
389389
return nil, err
390390
}
391391

392-
return s.client.Do(req.WithContext(ctx), nil)
392+
return s.client.Do(ctx, req, nil)
393393
}
394394

395395
// CreateImpersonation creates an impersonation OAuth token.
@@ -407,7 +407,7 @@ func (s *AuthorizationsService) CreateImpersonation(ctx context.Context, usernam
407407
}
408408

409409
a := new(Authorization)
410-
resp, err := s.client.Do(req.WithContext(ctx), a)
410+
resp, err := s.client.Do(ctx, req, a)
411411
if err != nil {
412412
return nil, resp, err
413413
}
@@ -426,5 +426,5 @@ func (s *AuthorizationsService) DeleteImpersonation(ctx context.Context, usernam
426426
return nil, err
427427
}
428428

429-
return s.client.Do(req.WithContext(ctx), nil)
429+
return s.client.Do(ctx, req, nil)
430430
}

0 commit comments

Comments
 (0)