Skip to content
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.

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

Make sure Context sets required fields. #1

Merged
merged 1 commit into from
Sep 25, 2016

Conversation

theckman
Copy link
Contributor

While working on getting the Go HTTP Routing Benchmark
to compile / run, an issue with the bear package was being observed.
Specifically, both the http.ResponseWriter and *http.Request sent
to the HandleFunc were set to nil. Upon digging in to the codebase
it was found that these two Context{} structs were being created
incorrectly.

It may be worth mentioning that I am not a user of bear, so I wouldn't
be able to determine whether these are the right changes to be making.
This should probably also have a regression test written for it.

Please let me know if any thing else is needed to merge this changeset.

While working on getting the [Go HTTP Routing Benchmark](https://github.com/julienschmidt/go-http-routing-benchmark)
to compile / run, an issue with the `bear` package was being observed.
Specifically, both the `http.ResponseWriter` and `*http.Request` sent
to the HandleFunc were set to `nil`. Upon digging in to the codebase
it was found that these two `Context{}` structs were being created
incorrectly.

It may be worth mentioning that I am not a user of `bear`, so I wouldn't
be able to determine whether these are the right changes to be making.
This should probably also have a regression test written for it.

Please let me know if any thing else is needed to merge this changeset.
@afshin
Copy link
Member

afshin commented Sep 25, 2016

Hi @theckman! Wow, you're totally right. I guess I haven't been using the root handler or the wildcard handlers with the signature that would have exposed this.

Thanks for fixing it; well spotted!

@afshin afshin merged commit 00ea705 into ursiform:master Sep 25, 2016
@afshin afshin changed the title make sure Context sets required fields Make sure Context sets required fields. Sep 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants