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

📝 Update: Add JSONDecoder to config #1489

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

pd-pranay
Copy link
Contributor

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?
Add json decoder to config

This pull request is in reference to #1452
Commit formatting

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

@pd-pranay
Copy link
Contributor Author

Let me know if everything is fine.

@ReneWerner87
Copy link
Member

Could you please add a test for the configuration possibility and a part in the docs repository.

@pd-pranay
Copy link
Contributor Author

Could you please add a test for the configuration possibility and a part in the docs repository.

Sure 👍

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

See the possibility to define the decoder, but no use within the framework/library, then shouldn't we use somewhere the method which was configured?

@pd-pranay
Copy link
Contributor Author

See the possibility to define the decoder, but no use within the framework/library, then shouldn't we use somewhere the method which was configured?

Yes, I was thinking to add this to methods in ctx.go with using app.config's Encoder and Decoder.
Will this do the work ? @ReneWerner87

func (c *Ctx)JSONDecoder(raw []byte, data interface{}) error {
	err := c.app.config.JSONDecoder(raw, data)
	if err != nil {
		return err
	}
	return nil
}
func (c *Ctx)JSONEncoder(data interface{}) ([]byte, error) {
	raw, err := c.app.config.JSONEncoder(data)
	if err != nil {
		return nil, err
	}
	return raw, nil
}

@ReneWerner87
Copy link
Member

thought rather that the decoder should be used in the bodyParser or everywhere where we directly use the json unmarshal

i wouldn't provide an extra method in the context because it doesn't really have any reference to it and the one who sets the decoders and encoders from the outside in the config can use them from the outside capsule too

the only benefit i can think of is that you exchange the internal usages

This was linked to issues Aug 18, 2021
@ReneWerner87
Copy link
Member

@pd-pranay could you change all the usages for the unmarshall method

@pd-pranay
Copy link
Contributor Author

@ReneWerner87 can you help me with references where I should be changing usages of unmarshal method ?
I found one is this correct one ?
https://github.com/gofiber/fiber/blob/master/ctx.go#L294

@ReneWerner87
Copy link
Member

@pd-pranay Yes, that should be the only

@pd-pranay
Copy link
Contributor Author

pd-pranay commented Aug 18, 2021

For docs what type should I define JSONDecoder ?

@ReneWerner87
Copy link
Member

https://docs.gofiber.io/api/fiber#config

it should just show up here so users know they can customize it, maybe you can add the doc for JSONEncode as well

@ReneWerner87 ReneWerner87 self-requested a review August 18, 2021 12:25
@ReneWerner87 ReneWerner87 merged commit 670170f into gofiber:master Aug 18, 2021
@ReneWerner87
Copy link
Member

https://docs.gofiber.io/api/fiber#config

it should just show up here so users know they can customize it, maybe you can add the doc for JSONEncode as well

@pd-pranay can you add JSONEncode and JSONDecode to the documentation ?
https://github.com/gofiber/docs/blob/master/api/fiber.md#config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 JSONDecoder to config Custom marshalers 🚀
2 participants