-
Notifications
You must be signed in to change notification settings - Fork 49
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
New serialization approach for the RequestBuilder #226
New serialization approach for the RequestBuilder #226
Conversation
Codecov Report
@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 86.99% 91.31% +4.32%
==========================================
Files 68 49 -19
Lines 2552 2095 -457
==========================================
- Hits 2220 1913 -307
+ Misses 332 182 -150 Continue to review full report at Codecov.
|
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!
@@ -121,8 +121,7 @@ | |||
], | |||
'MethodWithSpecialJsonMapping' => [ | |||
'method' => 'get', | |||
'uriTemplate' => '/v1/special/mapping', | |||
'placeholders' => [] | |||
'uriTemplate' => '/v1/special/mapping' |
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.
I don't understand how this change fits with the rest of the changes - do we need to remove placeholders
? Or is it just cleanup?
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.
It was just cleanup, I can undo if you like. Let me know!
@dwsupplee protobuf 3.7.0 is out! https://github.com/protocolbuffers/protobuf/releases/tag/v3.7.0 Can we more this PR forward? |
Absolutely 😄 |
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.
This looks great!
## Google ApiCore 1.0.0 Moving ApiCore to GA! ### RequestBuilding serialization approach - Added a new serialization approach in RequestBuilder (#226) ### Protobuf changes - Updated protobuf dependency to 3.7.1 (#232) - Added conflict section to composer.json to prevent protobuf extension conflict (#232) - Remove skip tests for protobuf extension (#230) ### Cleanup and dependencies - Update common-protos dependency to 1.0 (#231) - Update license files (#228)
## Google ApiCore 1.0.0 Moving ApiCore to GA! ### RequestBuilding serialization approach - Added a new serialization approach in RequestBuilder (#226) ### Protobuf changes - Updated protobuf dependency to 3.7.1 (#232) - Added conflict section to composer.json to prevent protobuf extension conflict (#232) - Remove skip tests for protobuf extension (#230) ### Cleanup and dependencies - Update common-protos dependency to 1.0 (#231) - Update license files (#228)
Addresses: googleapis/google-cloud-php#1583
This approach relies on
Google\Protobuf\Internal\Message::serializeToJsonString()
as opposed to accessing private/protected variables on protobuf messages. Thanks to some great work over the last few weeks by @TeBoring and the rest of the protobuf team, we will be able to safely rely on this simpler approach with the next release of the protobuf extension.