Conversation
ctx is a common and well understood variable name that represents the current context.Context value. This is a followup to #529.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
LGTM - just a couple minor comments that you can ignore if you wish.
| &oauth2.Token{AccessToken: "... your access token ..."}, | ||
| ) | ||
| tc := oauth2.NewClient(oauth2.NoContext, ts) | ||
| tc := oauth2.NewClient(context.Background(), ts) |
There was a problem hiding this comment.
What about adding a line before this:
ctx := context.Background()
and then changing this line to use ctx ?
There was a problem hiding this comment.
That seems reasonable. Edit: Done.
| &oauth2.Token{AccessToken: "... your access token ..."}, | ||
| ) | ||
| tc := oauth2.NewClient(oauth2.NoContext, ts) | ||
| tc := oauth2.NewClient(context.Background(), ts) |
There was a problem hiding this comment.
Same comment here:
What about adding a line before this:
ctx := context.Background()
and then changing this line to use ctx ?
|
Merging. |
|
@shurcooL - did I mess something up here? I see It looks like maybe I didn't get a129861 but I'm not sure how I messed that up. Yikes. I don't like what I'm seeing: $ git branch -v
gen-accessors 388cd37 [ahead 5, behind 3] Move gen-accessors.go into github directory
i-531 8fa3a78 [ahead 2, behind 2] Address review feedback - require non-nil pull argument
* master a129861 [ahead 3, behind 2] Declare ctx on top, reuse it for 2 calls.
pr/561 a129861 Declare ctx on top, reuse it for 2 calls.My Here is my master branch log: $ glog | head
* a129861 2017-02-22 | Declare ctx on top, reuse it for 2 calls. (HEAD -> master, google/context-doc-updates, pr/561) [Dmitri Shuralyov]
* 4a23999 2017-02-21 | Update documentation with new context parameter. [Dmitri Shuralyov]
* 654d25d 2017-02-21 | Update remaining uses of oauth2.NoContext. [Dmitri Shuralyov]
* 23d6cb9 2017-02-20 | Add support for context package to all endpoints. (#529) [Dmitri Shuralyov]
* 2ec691a 2017-02-18 | examples/basicauth: Simplify comma-ok type assertion in if statement. (#558) [Dmitri Shuralyov]
* de9eb29 2017-02-18 | Remove unused Client.mostRecent. (#559) [Dmitri Shuralyov]
* 2a4b920 2017-02-18 | Remove deprecated identifiers. (#555) [Dmitri Shuralyov]
* 796decd 2017-02-18 | Require Go 1.7 and newer. (#554) [Dmitri Shuralyov]
* 82629e0 2017-02-17 | Use HTTPS for all developer.github.com links. (#551) [Kevin Burke]
* dfd20fd 2017-02-15 | Return nil error explicitly when there is no error. (#550) [Nick Spragg]I think that I didn't properly handle different branches in the PR. I'm going to stop and see if @shurcooL can verify the integrity of the current master before I make any more changes. |
Where did you see that? It definitely looks like your local repository is in a weird state. You should probably make a backup of your current local master branch, then reset your local master to match the upstream master (so that it's not both behind and ahead). I can see that the remote master has 2 new commits since last night. They are 93f95bf and 2806581.
So this PR is fully merged in 2806581, and we can close it. You should resolve your local master branch issues, however. Also, thanks for showing me that |
|
Oh, I see what you were talking about regarding "Closed with unmerged commits". It only shows up when the PR is closed:
I think it's fine. I think it's because you merged the PR by squashing these 3 commits into one and closed it via commit message (the |
|
Thank you, @shurcooL! I appreciate the analysis and confirmation. |
oauth2.NoContext is deprecated. Replace it with context.Background() in a few more places that were missed in 23d6cb9. Related to google#560. Closes google#561. Change-Id: I17275ef6f5d2bc4ff0e59d58a66b54acea4233f7
2 participants
This is a followup to #529.