Adds ProtoJSON support for protobuf inputs #192
Conversation
| module Converter | ||
| module Payload | ||
| class ProtoJSON | ||
| class InvalidPayload < RuntimeError; end |
There was a problem hiding this comment.
Doesn't look like this is used anywhere. Should we add error handling as mentioned in the TODO? :)
There was a problem hiding this comment.
d'oh ya was going to add handling, but then noticed the JSON deserializer didn't have anything explicit so didn't follow up. removed the error class.
| end | ||
|
|
||
| def to_payload(data) | ||
| return unless data&.is_a?(Google::Protobuf::MessageExts) |
There was a problem hiding this comment.
nit: no need for &. since is_a? is defined on a NilClass as well
| @@ -0,0 +1,22 @@ | |||
| require 'temporal/connection/converter/payload/json' | |||
There was a problem hiding this comment.
this is a wrong require, you need proto_json
| it 'converts' do | ||
| input = Temporal::Api::Common::V1::Payload.new( | ||
| metadata: { 'hello' => 'world' }, | ||
| data: 'hello world', |
There was a problem hiding this comment.
This data type won't get picked by the target converter, right? It's not a subclass of Google::Protobuf::MessageExts
There was a problem hiding this comment.
added comment. using Temporal::Api::Common::V1::Payload as the input given it's an available proto message. to_payload wraps it, so the result data is the json'd input w/ the metadata
There was a problem hiding this comment.
Ah, I see, makes sense. It might be a bit clearer if you were to pick a proto that has nothing to do with payloads. But the comment solves this issue
| module Converter | ||
| module Payload | ||
| class ProtoJSON | ||
| class InvalidPayload < RuntimeError; end |
There was a problem hiding this comment.
d'oh ya was going to add handling, but then noticed the JSON deserializer didn't have anything explicit so didn't follow up. removed the error class.
| end | ||
|
|
||
| def to_payload(data) | ||
| return unless data&.is_a?(Google::Protobuf::MessageExts) |
| @@ -0,0 +1,22 @@ | |||
| require 'temporal/connection/converter/payload/json' | |||
| it 'converts' do | ||
| input = Temporal::Api::Common::V1::Payload.new( | ||
| metadata: { 'hello' => 'world' }, | ||
| data: 'hello world', |
There was a problem hiding this comment.
added comment. using Temporal::Api::Common::V1::Payload as the input given it's an available proto message. to_payload wraps it, so the result data is the json'd input w/ the metadata
antstorm
left a comment
antstorm
left a comment
There was a problem hiding this comment.
Looks good, thank you for the PR
| it 'converts' do | ||
| input = Temporal::Api::Common::V1::Payload.new( | ||
| metadata: { 'hello' => 'world' }, | ||
| data: 'hello world', |
There was a problem hiding this comment.
Ah, I see, makes sense. It might be a bit clearer if you were to pick a proto that has nothing to do with payloads. But the comment solves this issue
3 participants
#191