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

Support param for field from Oneof definition. #621

Merged
merged 4 commits into from
Apr 27, 2018

Conversation

bonafideyan
Copy link
Contributor

when the param binds with a field defined by Oneof, it can not be assigned directly.

Copy link
Member

@yugui yugui left a comment

Choose a reason for hiding this comment

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

Thank you for your great contribution. Almost LGTM except minor comments.

return
}
if got, want := msg.GetStatus().GetNote(), "golang"; got != want {
t.Errorf("msg.Lang = %q; want %q", got, want)
Copy link
Member

Choose a reason for hiding this comment

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

"msg.GetStaus().GetNote() = %q; want %q".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return
}
if got, want := msg.GetNo().GetNote(), "golang"; got != want {
t.Errorf("msg.Lang = %q; want %q", got, want)
Copy link
Member

Choose a reason for hiding this comment

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

"msg.GetNo().GetNote() = %q; want %q"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

int64 line_num = 3;
string lang = 4;
}
int64 time = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove time if you are not using it?
I'd like to keep SimpleMessage as simple as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bonafideyan
Copy link
Contributor Author

I didn't know those files need be regenerated and reason of build failure.
Fixed, thank you.

@codecov-io
Copy link

Codecov Report

Merging #621 into master will decrease coverage by 0.24%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage   59.12%   58.88%   -0.25%     
==========================================
  Files          30       30              
  Lines        2838     2853      +15     
==========================================
+ Hits         1678     1680       +2     
- Misses        998     1010      +12     
- Partials      162      163       +1
Impacted Files Coverage Δ
protoc-gen-grpc-gateway/descriptor/types.go 49.07% <31.57%> (-5.77%) ⬇️

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 639b2fa...f649150. Read the comment docs.

@yugui
Copy link
Member

yugui commented Apr 27, 2018

LGTM. Thank you for your contribution.

@yugui yugui merged commit 634904c into grpc-ecosystem:master Apr 27, 2018
@yugui yugui mentioned this pull request Apr 30, 2018
1 task
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* Support param for field from Oneof definition.

* Fix error reason and regenerate files.

* Regenerate echo_service.pb.gw.go.
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.

4 participants