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

feat: ShouldBindBodyWith shortcut and change doc #3871

Merged
merged 5 commits into from
Mar 23, 2024

Conversation

RedCrazyGhost
Copy link
Contributor

@RedCrazyGhost RedCrazyGhost commented Mar 6, 2024

Resolves #3823

Based on this #3823 proposal, I code about shortcuts

Ran into an issue like #3862 during testing

I hope to get some help.

This is my first time submitting code in open source。

@RedCrazyGhost
Copy link
Contributor Author

RedCrazyGhost commented Mar 12, 2024

I looked it up, and Red Hat says that YAML is a superset of JSON, and all the data can be parsed in JSON format.

In issue , a user told me that parsing is normal behavior

I set out to fix the test case.

@RedCrazyGhost
Copy link
Contributor Author

Hello, please take a look at the PR I submitted and give me some feedback. This PR provides 4 types of shortcuts to ShouldBindBodyWith, which are JSON, YAML, XML, and TOML.

Take a look at this PR, please. @appleboy @thinkerou @manucorporat

@appleboy appleboy added this to the v1.10 milestone Mar 13, 2024
@appleboy
Copy link
Member

I will take a look.

@appleboy
Copy link
Member

@RedCrazyGhost Please help to fix lint error.

@RedCrazyGhost
Copy link
Contributor Author

@RedCrazyGhost Please help to fix lint error.

@appleboy Thanks for your help, I am trying to fix the error in lint, I have committed fix commit.

@RedCrazyGhost
Copy link
Contributor Author

Hello, can you help me start the approval flow? @flc1125

@flc1125
Copy link
Contributor

flc1125 commented Mar 19, 2024

Hello, can you help me start the approval flow? @flc1125

I'm not a project member and can't help you. Thanks~

@RedCrazyGhost
Copy link
Contributor Author

Hello, can you help me start the approval flow? @flc1125

I'm not a project member and can't help you. Thanks~

Okay, I see. Sorry to bother you.

@RedCrazyGhost
Copy link
Contributor Author

@appleboy Hello, please approve the workflow if you have time

context_test.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.18%. Comparing base (3dc1cd6) to head (d7acf06).
Report is 30 commits behind head on master.

Files Patch % Lines
context.go 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3871      +/-   ##
==========================================
- Coverage   99.21%   99.18%   -0.03%     
==========================================
  Files          42       43       +1     
  Lines        3182     2709     -473     
==========================================
- Hits         3157     2687     -470     
+ Misses         17       12       -5     
- Partials        8       10       +2     
Flag Coverage Δ
?
-race ∅ <ø> (?)
-tags "sonic avx" 99.18% <80.00%> (?)
-tags go_json 99.18% <80.00%> (?)
-tags nomsgpack 99.17% <80.00%> (?)
go-1.18 99.11% <80.00%> (-0.01%) ⬇️
go-1.19 99.18% <80.00%> (-0.03%) ⬇️
go-1.20 99.18% <80.00%> (-0.03%) ⬇️
go-1.21 99.18% <80.00%> (-0.03%) ⬇️
go-1.22 99.18% <80.00%> (?)
macos-latest 99.18% <80.00%> (-0.03%) ⬇️
ubuntu-latest 99.18% <80.00%> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RedCrazyGhost
Copy link
Contributor Author

Do I need to pay attention to the CodeCov bot detection report?

Adding new method shortcuts results in a substantial drop in coverage.
@appleboy

@RedCrazyGhost
Copy link
Contributor Author

This codecov detection result is not the same as the result of go test coverage.

This makes me very confused!

I don't know how to solve this patch coverage? @appleboy

termial use code:

$ go test -coverprofile=coverage.out ./...
$ go tool cover -html=coverage.out
image

@appleboy appleboy merged commit 7a865dc into gin-gonic:master Mar 23, 2024
53 of 55 checks passed
@appleboy
Copy link
Member

@RedCrazyGhost I've checked the tests myself, so I won't refer to the data from codecov. :) Thanks.

@RedCrazyGhost
Copy link
Contributor Author

@RedCrazyGhost I've checked the tests myself, so I won't refer to the data from codecov. :) Thanks.

Thank you for your support throughout.

@RedCrazyGhost RedCrazyGhost deleted the feat-addNewFunction branch March 23, 2024 14:17
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.

ShouldBindBodyWith() will more types be supported in the future?
3 participants