Skip to content

Render subpkg with better payload control#173

Merged
pkieltyka merged 4 commits intomasterfrom
render
Mar 30, 2017
Merged

Render subpkg with better payload control#173
pkieltyka merged 4 commits intomasterfrom
render

Conversation

@pkieltyka
Copy link
Copy Markdown
Member

Rewrite of chi/render pkg, introducing Binder and Renderer interfaces for handling well-defined request/response payloads. See _examples/rest/main.go in this branch for a feel of the API.

See #84

Feedback is welcome!

… for handling well-defined request/response payloads
Copy link
Copy Markdown
Contributor

@c2h5oh c2h5oh left a comment

Choose a reason for hiding this comment

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

I like it - I just have a couple of questions/suggestions.

We should also remove some of the hardcoded values before we merge this - like for example charset of json response

Comment thread render/content_type.go
return ContentTypeEventStream
default:
return ContentTypeUnknown
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we used a global map[string]ContentType var we could let people add their own - similar to https://golang.org/pkg/mime/#AddExtensionType

Comment thread render/content_type.go

return ContentTypePlainText // Default ContentType.
if contentType == ContentTypeUnknown {
contentType = ContentTypePlainText
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's return ContentTypeUnknown instead of silently overwriting it

Comment thread render/render.go
func renderer(w http.ResponseWriter, r *http.Request, v Renderer) error {
rv := reflect.ValueOf(v)
if rv.Kind() == reflect.Ptr {
rv = rv.Elem()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My reflect knowledge is somewhat lacking: why not simply v = reflect.Indirect(reflect.ValueOf(v)) ?

Comment thread render/decoder.go
// itself. Effectively, allowing you to easily add your own logic to the package
// defaults. For example, maybe you want to impose a limit on the number of
// bytes allowed to be read from the request body.
var Decode = DefaultDecoder
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Being able to set a Decoder per Binder (with fallback to DefaultDecoder) would be super useful for handling forms with something like http://www.gorillatoolkit.org/pkg/schema

Comment thread render/render.go
JSON(w, r, v)
}
type Binder interface {
Bind(r *http.Request) error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See decoder comment

@pkieltyka pkieltyka merged commit e6033ea into master Mar 30, 2017
@pkieltyka pkieltyka deleted the render branch March 30, 2017 18:41
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