Skip to content

Store vars and route in context.Context when go1.7+ is used#169

Merged
kisielk merged 2 commits intogorilla:masterfrom
ejholmes:go1.7-context
Jun 4, 2016
Merged

Store vars and route in context.Context when go1.7+ is used#169
kisielk merged 2 commits intogorilla:masterfrom
ejholmes:go1.7-context

Conversation

@ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Jun 4, 2016

Fixes #168

This should be backwards compatible, since the usage of gorilla/context was an implementation detail and not exposed publicly.

When go1.7+ is used, the native context.Context object on the http.Request will be used. On older version of go, gorilla/context will be used.

@ejholmes ejholmes mentioned this pull request Jun 4, 2016
Closed
@elithrar
Copy link
Contributor

elithrar commented Jun 4, 2016

This LGTM! If @kisielk is happy with the PR as well we can merge.

@kisielk
Copy link
Contributor

kisielk commented Jun 4, 2016

I was actually thinking of doing that this weekend, but I guess you beat me to it !

The implementation looks great to me, very elegant.

The only thing I can think of it breaking is people who were manually extracting route vars via the gorilla/context package, but nobody should be doing that...

@kisielk kisielk merged commit e84fac9 into gorilla:master Jun 4, 2016
@elithrar
Copy link
Contributor

elithrar commented Jun 4, 2016

@kisielk Thankfully we didn't expose the context keys in #167 or #112!

Thanks for merging as well.

@kisielk
Copy link
Contributor

kisielk commented Jun 4, 2016

Yeah, that was explicitly for this reason, so we could change the context
storage method in the future.
On Sat, Jun 4, 2016 at 09:44 Matt Silverlock notifications@github.com
wrote:

@kisielk https://github.com/kisielk Thankfully we didn't expose the
context keys in #167 #167 or #112
#112!

Thanks for merging as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#169 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADk-iRGK5Es4kHI90_ZYdNkAJZId7V3ks5qIatigaJpZM4IuFga
.

@owenthereal
Copy link
Contributor

owenthereal commented Jul 30, 2016

👍 Was thinking of doing this.

@jordan-wright jordan-wright mentioned this pull request Sep 11, 2016
Closed
2 tasks
@grim-fendango grim-fendango mentioned this pull request Dec 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ejholmes @elithrar @kisielk @owenthereal