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

Allow for custom JSON encoding implementations #1880

Merged
merged 3 commits into from
Jul 5, 2021
Merged

Allow for custom JSON encoding implementations #1880

merged 3 commits into from
Jul 5, 2021

Conversation

hoshsadiq
Copy link
Contributor

@hoshsadiq hoshsadiq commented May 23, 2021

This PR is an alternative implementation to #1774.

It allows users to give a custom JSON encoder implementation including jsoniter as has been requested multiple times. My use-case is slightly different, in that I wanted to use https://github.com/google/jsonapi as the default JSON implementation.

Happy to try and raise some examples in the cookbook for this if this gets approved/merged.

closes #1177
closes #1204
closes #1394
closes #1698
closes #1774

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #1880 (f27677a) into master (2acb24a) will increase coverage by 0.75%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1880      +/-   ##
==========================================
+ Coverage   90.21%   90.97%   +0.75%     
==========================================
  Files          31       32       +1     
  Lines        2770     2791      +21     
==========================================
+ Hits         2499     2539      +40     
+ Misses        173      161      -12     
+ Partials       98       91       -7     
Impacted Files Coverage Δ
context.go 86.93% <40.00%> (-0.26%) ⬇️
bind.go 89.41% <100.00%> (+0.19%) ⬆️
echo.go 93.82% <100.00%> (+2.18%) ⬆️
json.go 100.00% <100.00%> (ø)
middleware/rate_limiter.go 100.00% <0.00%> (ø)
router.go 95.69% <0.00%> (+0.66%) ⬆️
middleware/request_id.go 84.21% <0.00%> (+1.85%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2acb24a...f27677a. Read the comment docs.

@aldas aldas added this to the v4.4.0 milestone Jun 2, 2021
Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@aldas
Copy link
Contributor

aldas commented Jun 2, 2021

This will go in next minor v4.4 release.

@aldas aldas mentioned this pull request Jun 2, 2021
Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

LGTM too!

@lammel
Copy link
Contributor

lammel commented Jun 12, 2021

@hoshsadiq Nice work! Could you also add a PR in github.com/labstack/echox to update the docs. I'd love to see an example using Jsoniter there.

This PR is currently about JSON encoding only. Should we consider decoding JSON too?
Especially when dealing with larger POSTS with JSON body that might be of interest for the binder.

@hoshsadiq
Copy link
Contributor Author

Will raise the PR in echox soon.

I was thinking about the decoder, I just couldn't think where it would be needed. Not sure why. But now it makes sense. I'll update the PR soon.

@hoshsadiq hoshsadiq requested review from aldas and pafuent June 13, 2021 13:56
@hoshsadiq
Copy link
Contributor Author

I've just pushed a new commit that also adds the decoder. Wasn't quite sure what to name the interface, so if you have a better name for it, feel free to suggest!

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@lammel
Copy link
Contributor

lammel commented Jun 13, 2021

I've just pushed a new commit that also adds the decoder. Wasn't quite sure what to name the interface, so if you have a better name for it, feel free to suggest!

I'd like to take the chance to discuss on naming of the interface, as the name should not change anytime soon after that (and will stick probably with v5 too). I personally like JSONCodec as it is pretty descriptive on that the interface is about.

Some other suggestions:

  • JSONCoder ... more Go-like
  • JSONSerializer ... descirbing the actual purpose of serialization
  • JSONHandler ... pretty generic term

Other suggestions welcome, your feedback even more.

@hoshsadiq
Copy link
Contributor Author

Personally I like JSONHandler and JSONSerializer (in that order). If there is no one other suggestions in the next few days, I'll change it to be JSONHandler.

@pafuent
Copy link
Contributor

pafuent commented Jun 15, 2021

In my case I prefer an explicit name, even if it is longer, like io.ReadWriteCloser because it is self explanatory. So, for me, I prefer JSONEncodeDecoder

@lammel
Copy link
Contributor

lammel commented Jun 16, 2021

In my case I prefer an explicit name, even if it is longer, like io.ReadWriteCloser because it is self explanatory. So, for me, I prefer JSONEncodeDecoder

That would lead to JSONWriter and JSONReader being to separate interfaces with a JSONReadWriter combining them.
But something like JSONSerializer oder JSONCoder does also describe the intended behaviour (which is the point of interfaces for me).

(That is no real objection, more a note of slight distaste for too long interface names)

@aldas
Copy link
Contributor

aldas commented Jun 30, 2021

ping, I vote for JSONSerializer and lets get this PR merged :)

@lammel
Copy link
Contributor

lammel commented Jun 30, 2021

I change my vote also to JSONSerializer, lets merge! ;-)
@hoshsadiq Please go ahead an rename. Thanks!

@aldas
Copy link
Contributor

aldas commented Jul 5, 2021

well well well. I was about to rename it myself and push changes but I started to look at that interface and JSONSerializer does not make sense with those method names. For encode and decode methods JSONCoder makes more sense.

	JSONCoder interface {
		Encode(c Context, i interface{}, indent string) error
		Decode(c Context, i interface{}) error
	}

@hoshsadiq
Copy link
Contributor Author

Sorry, haven't had a chance to do this. If it becomes JSONSerializer, the methods would be Serialize and Deserialize.

@aldas
Copy link
Contributor

aldas commented Jul 5, 2021

Makes sense 👍 . @hoshsadiq you want to do renaming part yourself or I'll do it - that PR is already open at my IDE.

@hoshsadiq
Copy link
Contributor Author

If you've got time feel free to do it, if not, I'll probably get to it on Sunday.

@aldas
Copy link
Contributor

aldas commented Jul 5, 2021

done. I'll merge it and in couple of days prepare changelog and release in v4.4.0

@aldas aldas merged commit 5e791b0 into labstack:master Jul 5, 2021
@hoshsadiq hoshsadiq deleted the custom-json branch October 3, 2021 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants