Skip to content

Comments

HTTP server NoOpRequestDecoder#659

Merged
peterbourgon merged 1 commit intogo-kit:masterfrom
jdolce:master
Feb 5, 2018
Merged

HTTP server NoOpRequestDecoder#659
peterbourgon merged 1 commit intogo-kit:masterfrom
jdolce:master

Conversation

@jdolce
Copy link
Contributor

@jdolce jdolce commented Feb 2, 2018

Adds NoOpRequestDecoder for HTTP request that do not need to be decoded.

Fixes #657


// NoOpRequestDecoder is a DecodeRequestFunc that can be used for requests that do not
// need to be decoded, and simply returns nil, nil.
func NoOpRequestDecoder(ctx context.Context, r *http.Request) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it might look a bit strange, but I think in Go we usually spell this NopRequestDecoder. See for precedent:

https://golang.org/pkg/io/ioutil/#NopCloser
https://godoc.org/github.com/go-kit/kit/log#NewNopLogger
https://godoc.org/golang.org/x/text/transform#NopResetter

case <-done:
case <-time.After(time.Second):
t.Fatal("timeout waiting for finalizer")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is doing too much. It seems to me that it would be perfectly valid to test this without spinning up an http server and extra goroutines. Consider calling handler.ServeHTTP directly. You can use httptest.NewRequest and httptest.NewRecorder to construct appropriate arguments for ServeHTTP and do it all sequentially with no need for worrying about timeouts, done channels or stopping a server.

Adds NoOpRequestDecoder for HTTP request that do not need to be decoded.

Fixes go-kit#657
@jdolce
Copy link
Contributor Author

jdolce commented Feb 5, 2018

@ChrisHines Thanks for the review. I made the requested changes. Cheers.

Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peterbourgon
Copy link
Member

Fun! Thanks! I guess we ought to do a release soon...

@peterbourgon peterbourgon merged commit 321a6f5 into go-kit:master Feb 5, 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.

4 participants