-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Stop marshalling any.Any types unnecessarily. #577
Conversation
762475a
to
e3b25f1
Compare
@achew22 this should be a harmless change, could you take a quick look please? |
My understanding of the proto registry is unfortunately lacking WRT this topic. My expectation is that the proto on the wire wouldn't contain enough information to serialize it down to json. Does the any proto contain a field => name mapping in the wire format for it or something? Could you also add a test to ensure this behavior doesn't regress? |
This changes just removes a no-op that was there from when this was first added, probably because the original implementation was confused as to whether this would be necessary or not. This kind of marshalling is already done in several other places: grpc-gateway/runtime/proto_errors.go Line 37 in 58f78b9
grpc-gateway/runtime/handler.go Line 178 in 58f78b9
I will look at adding some tests. |
Added a test for marshalling with details. |
@johanbrandhorst thanks for this, can you rebase and resolve that test conflict? |
9a9dad4
to
da583f8
Compare
@tmc rebase completed! |
@tmc Is that test failure due to my change? It seems unrelated. |
Sorry I missed your comment. The failure does appear to have been unrelated. Please resolve the conflicts here and let's get this in! |
Rebased :) |
12fb41f
to
efae803
Compare
Think I've hit a flaky test, could I have a rerun please? @tmc |
Retriggered |
Looking good on my end, happy to merge? |
Not entirely sure why it's required me to regenerate the protofiles though 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
To add ,proto3 |
@achew22 I'll let you complete review and merge |
LGTM. Looks like there is a merge conflict. It isn't trivial to do from the web interface and I'm at work, but @tmc feel free to merge if you can get it to have no conflicts |
Curses! I'll fix it. |
Fixes grpc-ecosystem#576. Adds a test case for marshalling of details as well.
Alright that should've done it. I had to pull and push over HTTPS as this airport WiFi must be blocking port 22 🤔. |
Codecov Report
@@ Coverage Diff @@
## master #577 +/- ##
==========================================
+ Coverage 56.43% 57.95% +1.51%
==========================================
Files 30 30
Lines 3005 2918 -87
==========================================
- Hits 1696 1691 -5
+ Misses 1145 1064 -81
+ Partials 164 163 -1
Continue to review full report at Codecov.
|
Fixes #576.