fasthttp
fasthttp copied to clipboard
valyala
(h *ResponseHeader) StatusCode() int Leads to misunderstanding
header.go
// StatusCode returns response status code.
func (h *ResponseHeader) StatusCode() int {
if h.statusCode == 0 {
return StatusOK
}
return h.statusCode
}
leads to misunderstanding, because while I debug my code, I expect StatusCode() will return the the value with same sense as h.statusCode but it returns HTTP response code. I propose to rename this method to HTTPStatusCode or assign to h.statusCode another name.
I agree that it would have been better if the if statusCode == 0 { wasn't in there. But I'm afraid we can't change this anymore without breaking backwards compatibility. We can't change the name of the method either.
Renaming statusCode would be possible as it isn't exposed. What would you suggest?
I think that we can leave the naming by make the function as
// StatusCode returns response status code.
func (h *ResponseHeader) StatusCode() int {
return h.statusCode
}
Because it is not ok that after the err := client.DoTimeout(&req, &resp, time.Millisecond) it returns 200 code. If you are agree with me - I can make pull request, but tests require changes it that case
Changing existing tests may hint that we might've broken backward compatibility. I doubt it's a good idea to change old public api behavior too much
On the one hand it would be a backwards incompatible change, on the other hand I can see how this could cause bugs. Not checking the error before the statusCode is of course the biggest bug here. But if people don't have that bug then making this change won't affect them.
But... I quickly tried to make the change and noticed that almost all tests fail. I'm not sure why statusCode is 0 so often when it should be 200. I also don't have the time to look into this.
So if you're going to make this change you'll need to change more code to make sure statusCode is 200 when expected.
This simple test for example should pass without modifying the test or verifyResponse:
func TestBasic(t *testing.T) {
s := &Server{
Handler: func(ctx *RequestCtx) {
ctx.WriteString("test")
},
}
rw := &readWriter{}
rw.r.WriteString("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n")
if err := s.ServeConn(rw); err != nil {
t.Fatalf("Unexpected error from serveConn: %s", err)
}
br := bufio.NewReader(&rw.w)
verifyResponse(t, br, 200, "text/plain; charset=utf-8", "test")
}
@erikdubbelboer so, I can do that. Is it significant and will be merged?
@erikdubbelboer I figured out that the main problem in missing of response status code in mocking server for tests. I added it