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

Expose http.Hijacker interface on Response #355

Merged
merged 2 commits into from Sep 21, 2017

Conversation

Projects
None yet
2 participants
@johnweldon
Contributor

johnweldon commented Sep 19, 2017

Addresses #354

johnweldon added some commits Sep 19, 2017

Use named fields in the Response struct initialization
This allow changes to the type without impacting existing usages.
Expose http.Hijacker
From underlying http.ResponseWriter, if available.
contentLength int // number of bytes written for the response body
prettyPrint bool // controls the indentation feature of XML and JSON serialization. It is initialized using var PrettyPrintResponses.
err error // err property is kept when WriteError is called
hijacker http.Hijacker // if underlying ResponseWriter supports it

This comment has been minimized.

@emicklei

emicklei Sep 21, 2017

Owner

is adding a field necessary? the Hijack() function can access the embbed ResponseWriter.

This comment has been minimized.

@johnweldon

johnweldon Sep 21, 2017

Contributor

The problem is that the embedded ResponseWriter is stored as the interface, and you lose access to the implementation that supports the Hijack() method. (You can see this by attempting to do a type assertion on the embedded ResponseWriter as a Hijacker - it will fail.

This comment has been minimized.

@emicklei

emicklei Sep 21, 2017

Owner

thanks for clarifying this.

@@ -49,7 +49,7 @@ func TestKeyValueEncoding(t *testing.T) {
// Write
httpWriter := httptest.NewRecorder()
// Accept Produces
resp := Response{httpWriter, "application/kv,*/*;q=0.8", []string{"application/kv"}, 0, 0, true, nil}
resp := Response{ResponseWriter: httpWriter, requestAccept: "application/kv,*/*;q=0.8", routeProduces: []string{"application/kv"}, prettyPrint: true}

This comment has been minimized.

@emicklei

emicklei Sep 21, 2017

Owner

thank you for making these changes; at the time of initial writing, I was lazy and then the arguments started growing...

@emicklei emicklei merged commit 5741799 into emicklei:master Sep 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johnweldon johnweldon deleted the MustWin:354-response-hijacker branch Sep 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment