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

Modular deps for binding and render #1856

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sapk
Copy link
Contributor

@sapk sapk commented Apr 17, 2019

Following the similar pattern as sql drivers.
I extract xml, json, msgpack, protobuf and yaml into separate module.
This PR may break compat with previous versions.

For example: https://github.com/sapk-fork/gin-examples/blob/test-modular/basic/main.go

I think we can maybe re-add json and xml as default.

Related #1847 (issue) & #1852 (previous non-breaking solution)

@sapk
Copy link
Contributor Author

sapk commented Apr 17, 2019

For details:

  • Context function calls are unchanged
  • If the need module is not loaded it will panic for method like (c *Context) MsgPack or (c *Context) BindYAML(obj interface{})

@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #1856 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1856      +/-   ##
==========================================
+ Coverage   98.65%   99.14%   +0.49%     
==========================================
  Files          41       30      -11     
  Lines        2149     1991     -158     
==========================================
- Hits         2120     1974     -146     
+ Misses         18       11       -7     
+ Partials       11        6       -5
Impacted Files Coverage Δ
gin.go 99.1% <ø> (ø) ⬆️
binding/uri.go 100% <100%> (ø) ⬆️
render/render_17.go 100% <100%> (ø)
render/render.go 100% <100%> (ø) ⬆️
binding/form.go 100% <100%> (ø) ⬆️
context.go 98.38% <100%> (ø) ⬆️
mode.go 100% <100%> (+9.09%) ⬆️
render/html.go 100% <100%> (ø) ⬆️
binding/query.go 100% <100%> (ø) ⬆️
render/reader.go 100% <100%> (ø) ⬆️
... and 6 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 202f8fc...fce78a6. Read the comment docs.

@sapk sapk force-pushed the modular-deps branch 3 times, most recently from 4f66657 to e472164 Compare April 17, 2019 21:13
@dmarkham
Copy link
Contributor

dmarkham commented Apr 18, 2019

@sapk 👏

For example: https://github.com/sapk-fork/gin-examples/blob/test-modular/basic/main.go

I kinda like the way this feels only opting into what you want really nice 👍

I think we can maybe re-add json and xml as default.

I do like the idea of the "builtins" being there by default. But would that allow the user to replace the xml/json with a different (faster) binding/render for json/xml if we are including it by default?

I'm really interested in the core teams thoughts on this idea is!

👍 on the user's interface.
I'm still looking at the code to get a better feel for how you pulled this off.

If everyone likes this direction have a few chores like Docs, examples, benchmark, alloc's, etc.

@sapk
Copy link
Contributor Author

sapk commented Apr 18, 2019

@dmarkham Yes you can load a module that fill the same id in the respective common List to be call in place of the gin one.
If you want to already use a custom binding or render this is already possible with context method MustBindWith/ShouldBindWith and Render that just need a object that provide the respective interfaces.

@dmarkham dmarkham mentioned this pull request May 6, 2019
@thinkerou thinkerou requested review from thinkerou, appleboy and javierprovecho and removed request for thinkerou, appleboy and javierprovecho May 7, 2019 14:52
@zzjin
Copy link
Contributor

zzjin commented May 10, 2019

any update discuss?

@thinkerou thinkerou mentioned this pull request Jun 2, 2019
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.

3 participants