-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Feature: configurable JSON marshaller #99
Conversation
go.mod
Outdated
@@ -5,6 +5,7 @@ go 1.16 | |||
require ( | |||
github.com/atreugo/mock v0.0.0-20200601091009-13c275b330b0 | |||
github.com/fasthttp/router v1.4.16 | |||
github.com/json-iterator/go v1.1.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @boatrainlsz,
Thank you so much for your contribution and for using atreugo!
The PR looks good, but i wouldn't want to add a new dependency for json marshaling (in this case).
So i suggest you just create an interface for marhshaling, add it to config type and create a default function that does the same as before your changes!
In this way, it's more configurable and extensible to any marshaler! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @boatrainlsz,
Thank you so much for your contribution and for using atreugo!
The PR looks good, but i wouldn't want to add a new dependency for json marshaling (in this case). So i suggest you just create an interface for marhshaling, add it to config type and create a default function that does the same as before your changes!
In this way, it's more configurable and extensible to any marshaler! 😉
@savsgio Thanks for your advice! I'll take a look.
jsonMarshaller := ctx.jsonMarshaller | ||
if jsonMarshaller == nil { | ||
jsonMarshaller = defaultJSONMarshaller | ||
} | ||
if jm, ok := body.(json.Marshaler); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all the block to the default marshaler function, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the code must be something like:
ctx.jsonMarshalFunc(body)
@@ -46,7 +50,8 @@ type Config struct { // nolint:maligned | |||
TLSEnable bool | |||
CertKey string | |||
CertFile string | |||
|
|||
// JSONMarshaller is used to marshal the response body to JSON. | |||
JSONMarshaller JSONMarshaller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, this field doesn't need to be an interface, could be simpler with something like this:
JSONMarshal func(v interface{}) ([]byte, error)
@@ -16,11 +16,14 @@ func (ctx *RequestCtx) JSONResponse(body interface{}, statusCode ...int) (err er | |||
} | |||
|
|||
var data []byte | |||
|
|||
jsonMarshaller := ctx.jsonMarshaller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a dead code, because the server handler always set the json marshaler configured in the server.
But you must set the default marshaler into AcquireRequestCtx() function
Closed by #105 |
add support for configurable JSON marshaller, Fix #98