Skip to content

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

Merged
thinkerou merged 1 commit into
gin-gonic:masterfrom
GitStartHQ:GIN-001
May 28, 2022
Merged

[GIN-001] - Add TOML bining for gin#3081
thinkerou merged 1 commit into
gin-gonic:masterfrom
GitStartHQ:GIN-001

Conversation

@Valentine-Mario
Copy link
Copy Markdown
Contributor

@Valentine-Mario Valentine-Mario commented Mar 15, 2022 •
edited
Loading

  • 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
Copy Markdown
Contributor Author

Valentine-Mario commented Mar 15, 2022

@appleboy can you please help review

@efectn
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

Valentine-Mario commented Mar 16, 2022

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

appleboy
appleboy requested changes Mar 16, 2022
View reviewed changes
Comment thread binding/toml_test.go Outdated
@appleboy appleboy added this to the v1.8 milestone Mar 16, 2022
@appleboy appleboy requested a review from thinkerou March 16, 2022 08:17
@efectn
Copy link
Copy Markdown

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
Copy Markdown
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
View reviewed changes
Copy link
Copy Markdown

@efectn efectn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

thinkerou
thinkerou reviewed Mar 17, 2022
View reviewed changes
Comment thread go.mod
efectn
efectn suggested changes Mar 17, 2022
View reviewed changes
Comment thread go.mod Outdated
appleboy
appleboy previously approved these changes Mar 17, 2022
View reviewed changes
@Valentine-Mario
Copy link
Copy Markdown
Contributor Author

Valentine-Mario commented Mar 18, 2022

@appleboy fixed issues with gofmt

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

appleboy commented Mar 19, 2022

Wait for @thinkerou approval.

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

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm, but please wait unit test ok, thanks!

@gitstart-app gitstart-app Bot dismissed stale reviews from thinkerou and appleboy via 32e4848 March 21, 2022 07:49
@Valentine-Mario
Copy link
Copy Markdown
Contributor Author

Valentine-Mario commented Mar 21, 2022 •
edited
Loading

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

cc @thinkerou @appleboy

@thinkerou
Copy link
Copy Markdown
Member

thinkerou commented Mar 21, 2022

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

@Valentine-Mario
Copy link
Copy Markdown
Contributor Author

Valentine-Mario commented Mar 22, 2022

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

Resolved @thinkerou

@Valentine-Mario
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

codecov Bot commented Apr 5, 2022 •
edited
Loading

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.

thinkerou
thinkerou reviewed May 28, 2022
View reviewed changes
Comment thread binding/toml.go
@@ -0,0 +1,31 @@
package binding
Copy link
Copy Markdown
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 reviewed May 28, 2022
View reviewed changes
Comment thread render/toml.go
@@ -0,0 +1,32 @@
package render
Copy link
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thinkerou thinkerou thinkerou left review comments

@appleboy appleboy appleboy approved these changes

+1 more reviewer

@efectn efectn efectn requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

v1.8

Development

Successfully merging this pull request may close these issues.

5 participants

@Valentine-Mario @efectn @appleboy @thinkerou @gitstart