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

Feature: configurable JSON marshaller #99

Closed
wants to merge 3 commits into from

Conversation

boatrainlsz
Copy link

add support for configurable JSON marshaller, Fix #98

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
Copy link
Owner

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! 😉

Copy link
Author

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.

@savsgio savsgio added the enhancement New feature or request label Feb 9, 2023
@savsgio savsgio assigned savsgio and unassigned savsgio Feb 9, 2023
@boatrainlsz
Copy link
Author

@savsgio Hi, I just made another commit 79b1abf, please kindly review it! 😃

jsonMarshaller := ctx.jsonMarshaller
if jsonMarshaller == nil {
jsonMarshaller = defaultJSONMarshaller
}
if jm, ok := body.(json.Marshaler); ok {
Copy link
Owner

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

Copy link
Owner

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
Copy link
Owner

@savsgio savsgio Feb 13, 2023

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
Copy link
Owner

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

@savsgio
Copy link
Owner

savsgio commented Oct 18, 2023

Closed by #105

@savsgio savsgio closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable JSON Marshaller
2 participants