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

[GIN-001] - Add TOML bining for gin #3081

Merged
merged 1 commit into from May 28, 2022
Merged

Conversation

Valentine-Mario
Copy link
Contributor

@Valentine-Mario Valentine-Mario commented Mar 15, 2022

  • With pull requests:
    • Open your pull request against master
    • Your pull request should have no more than two commits, if not you should squash them.
    • It should pass all tests in the available continuous integration systems such as GitHub Actions.
    • You should add/modify tests to cover your proposed code changes.
    • If your pull request contains a new feature, please document it on the README.

This PR aims to add a TOML binding to the gin framework

@Valentine-Mario Valentine-Mario changed the title [GIN-001] - Add TOML bining for gi [GIN-001] - Add TOML bining for gin Mar 15, 2022
@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 15, 2022

@appleboy can you please help review

@efectn
Copy link

efectn commented Mar 16, 2022

I think you should use v2 of github.com/pelletier/go-toml. It's more performant

@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 16, 2022

The v2 version is still in beta and might still have numerous bugs at the moment

binding/toml_test.go Outdated Show resolved Hide resolved
@appleboy appleboy added this to the v1.8 milestone Mar 16, 2022
@appleboy appleboy requested a review from thinkerou Mar 16, 2022
@efectn
Copy link

efectn commented Mar 16, 2022

The v2 version is still in beta and might still have numerous bugs at the moment

Hugo uses v2, also i think it doesn't have critical bugs.
gohugoio/hugo@a3701e0

@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 17, 2022

The v2 version is still in beta and might still have numerous bugs at the moment

Hugo uses v2, also i think it doesn't have critical bugs. gohugoio/hugo@a3701e0

The TOML parser has been updated to v2. Please review. Thanks

efectn
efectn previously approved these changes Mar 17, 2022
Copy link

@efectn efectn left a comment

Looks good to me.

go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
appleboy
appleboy previously approved these changes Mar 17, 2022
@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 18, 2022

@appleboy fixed issues with gofmt

appleboy
appleboy previously approved these changes Mar 19, 2022
@appleboy
Copy link
Member

appleboy commented Mar 19, 2022

Wait for @thinkerou approval.

thinkerou
thinkerou previously approved these changes Mar 20, 2022
Copy link
Member

@thinkerou thinkerou left a comment

lgtm, but please wait unit test ok, thanks!

@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 21, 2022

Resolved merge conflict and updated toml binding to use the type any

cc @thinkerou @appleboy

@thinkerou
Copy link
Member

thinkerou commented Mar 21, 2022

@Valentine-Mario sorry, please fix conflicts again, thanks!

@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 22, 2022

@Valentine-Mario sorry, please fix conflicts again, thanks!

Resolved @thinkerou

@Valentine-Mario
Copy link
Contributor Author

Valentine-Mario commented Mar 23, 2022

@thinkerou not sure why this test is failing, com planning about the TOML in the context being undefined

@efectn
Copy link

efectn commented Apr 2, 2022

@thinkerou not sure why this test is failing, com planning about the TOML in the context being undefined

Check https://github.com/gin-gonic/gin/blob/master/binding/binding_nomsgpack.go

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #3081 (447e325) into master (c458094) will decrease coverage by 0.55%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3081      +/-   ##
==========================================
- Coverage   98.77%   98.21%   -0.56%     
==========================================
  Files          41       43       +2     
  Lines        3105     3141      +36     
==========================================
+ Hits         3067     3085      +18     
- Misses         26       44      +18     
  Partials       12       12              
Flag Coverage Δ
go-1.14 98.21% <50.00%> (-0.56%) ⬇️
go-1.15 98.05% <50.00%> (-0.56%) ⬇️
go-1.16 98.05% <50.00%> (-0.54%) ⬇️
go-1.17 97.96% <50.00%> (-0.56%) ⬇️
go-1.18 97.96% <50.00%> (-0.56%) ⬇️
macos-latest 98.21% <50.00%> (-0.56%) ⬇️
nomsgpack 98.19% <47.05%> (-0.57%) ⬇️
ubuntu-latest 98.21% <50.00%> (-0.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
render/render.go 100.00% <ø> (ø)
render/toml.go 0.00% <0.00%> (ø)
context.go 96.74% <22.22%> (-1.17%) ⬇️
binding/binding.go 100.00% <100.00%> (ø)
binding/binding_nomsgpack.go 100.00% <100.00%> (ø)
binding/toml.go 100.00% <100.00%> (ø)

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 c458094...447e325. Read the comment docs.

@@ -0,0 +1,31 @@
package binding
Copy link
Member

@thinkerou thinkerou May 28, 2022

Choose a reason for hiding this comment

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

please add copyright info, thanks!

@@ -0,0 +1,32 @@
package render
Copy link
Member

@thinkerou thinkerou May 28, 2022

Choose a reason for hiding this comment

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

please add copyright info, thanks!

@thinkerou thinkerou merged commit ed03102 into gin-gonic:master May 28, 2022
24 of 26 checks passed
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.

None yet

5 participants