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

Add support for Protobuf format response and unit test #1479

Merged
merged 25 commits into from
Aug 19, 2018

Conversation

salamer
Copy link
Contributor

@salamer salamer commented Aug 12, 2018

Gin now have the protobufBinding function to check the request format, but didn't have a protobuf response function like c.YAML().
In our company ByteDance, the largest internet company using golang in China, we use gin to transfer Protobuf instead of Json, we have to write some internal library to make some wrappers to achieve that, and the code is not elegant. So we really want such a feature.

@codecov
Copy link

codecov bot commented Aug 12, 2018

Codecov Report

Merging #1479 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1479      +/-   ##
==========================================
+ Coverage   99.15%   99.16%   +<.01%     
==========================================
  Files          36       37       +1     
  Lines        1894     1905      +11     
==========================================
+ Hits         1878     1889      +11     
  Misses         13       13              
  Partials        3        3
Impacted Files Coverage Δ
render/render.go 100% <ø> (ø) ⬆️
context.go 99.49% <100%> (ø) ⬆️
render/protobuf.go 100% <100%> (ø)

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 f856aa8...47a973a. Read the comment docs.

@salamer
Copy link
Contributor Author

salamer commented Aug 12, 2018

Oh my god, the codecov is too strict 😂

@thinkerou
Copy link
Member

@salamer please add some test case, thanks a lot!

context_test.go Outdated
@@ -17,11 +17,12 @@ import (
"testing"
"time"

"io"
Copy link
Member

Choose a reason for hiding this comment

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

order by first character.

@salamer
Copy link
Contributor Author

salamer commented Aug 15, 2018

I'm sorry, I'm too busy these days.
I will add some tests in this weekend. and can I generate a single proto file instead of using the proto from grpc exmaple

salamer and others added 17 commits August 18, 2018 11:07
mkdir a test data dir.
"not match neither" means that it will match.
* add issue/pr template explain

* add issue/pr template explain
…-gonic#1478)

* docs: add changelog for v1.3.0, update authors and version const

*  add link for every referenced pull request (gin-gonic#1481)

* docs: add changelog for v1.3.0, update authors and version const

* add link for pr
upgrade lib version, and upgrade `github.com/json-iterator/go` to add two libs.
@salamer
Copy link
Contributor Author

salamer commented Aug 18, 2018

Oh my god, the 99.15% coverage is too strict. I have already filled all my new function test. could it be a little bit lower?
And thanks for the test data.

context_test.go Outdated
protoData, err := proto.Marshal(data)
assert.NoError(t, err)

assert.Equal(t, 201, w.Code)
Copy link
Member

Choose a reason for hiding this comment

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

Please change 201 to http status code in net/http package.

// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

Copy link
Member

Choose a reason for hiding this comment

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

Please change to

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

fixed missed code bug

replace 201 with http lib and update annotation
@salamer
Copy link
Contributor Author

salamer commented Aug 18, 2018

@appleboy Hi, I have already go pass all the test and update my code as what you commented.
I wanna know would it be a feature for Gin v1.4? I think maybe I can add some example in the README

@appleboy
Copy link
Member

@salamer Yes. It would be a feature in v1.4 now. Please add an example in README.

@salamer
Copy link
Contributor Author

salamer commented Aug 18, 2018

@appleboy @thinkerou Hi, I'm sorry I'm too busy these days, I can only submit code intermittently 😔. and I have already finished this feature and add clear example and annotation. I hope it can be merged😂😂

@appleboy
Copy link
Member

@thinkerou need your approval.

@thinkerou
Copy link
Member

@appleboy done

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.