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

Write status code in external writer too #625

Closed
wants to merge 1 commit into from

Conversation

marvell
Copy link

@marvell marvell commented May 24, 2016

No description provided.

@coveralls
Copy link

coveralls commented May 24, 2016

Coverage Status

Coverage increased (+0.004%) to 94.088% when pulling dd48c05 on marvell:develop into 542be2f on gin-gonic:develop.

@heyimalex
Copy link

Are you wrapping the ResponseWriter with your own? Because I'm pretty sure Context.Writer and Context.writermem are the same object, one is just wrapped up in an interface.

@erizocosmico
Copy link

erizocosmico commented Dec 7, 2016

@heyimalex but if you change the Context.Writer, which is possible and middlewares like gin cache do, Context.writermem is not changed. And some things are only written in the private one, like the status https://github.com/gin-gonic/gin/blob/master/context.go#L378-L380

If you change Context.Writer you have to do this in order to set the status, which is hackish and ugly:

ctx.Writer.WriteHeader(http.StatusOK)
ctx.Writer.WriteHeaderNow()
// then write the response

@heyimalex
Copy link

If you change Context.Writer, don't you need to wrap the old one? If you wanted the wrapping-writers to get the status, then this method should only call Context.Writer.WriteHeader and not the one on writermem.

@willnewrelic
Copy link
Contributor

I'm with @heyimalex . Shouldn't this function simply be:

func (c *Context) Status(code int) {
	c.Writer.WriteHeader(code)
}

@periclesroo
Copy link

Hey, any updates on this one?

@thinkerou
Copy link
Member

@marvell please re-commit the pull request to master branch, thanks!

@willnewrelic
Copy link
Contributor

@thinkerou @heyimalex @marvell

I have opened a new PR against latest master with this change fixed:

#1606

@thinkerou thinkerou closed this Oct 24, 2018
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.

7 participants