Closed drmaas closed 5 years ago
@jprobinson more background:
At Target we have specs that use maps, of which some have value types that are lists. Currently we are unable to transpile our specs with these maps into protobuf schemas. This provides a mechanism to provide the conversion. This does not change how non-map lists are described in the protobuf schema.
The rationale for the generateRpcs
flag is more of a convenience to be able to generate schemas for use outside of grpc.
The rationale for prefixEnums
is that our specs often have the same enum values in different schemas, but inside of the same package (the package is specified by the spec title, and since a single protobuf schema file with a single package is generated, the enum values are not namespaced). Adding a command line option for this allows us to transpile our specs.
Thanks for the explanation! I personally use this tool for non gRPC services so I can understand the want to clean up some of the RPC stuff in the proto spec.
Would you mind altering existing test fixtures or providing new ones to cover these scenarios?
@jprobinson yes I will do that. Also - I am looking into generating custom wrapper messages instead of ListValue
. As you pointed out, it's untyped, which will be difficult to work with.
@jprobinson please re-review - the map lists are now statically typed and other suggestions have been implemented. I didn't add any tests yet, and can add those if desired.
I ran this locally against a 6400 line spec, and compiled java code from the protobuf schema, so I am fairly certain it works as intended.
Example of how this looks in protobuf:
message CartItemInsertRequest {
...
string add_mode = 1;
map<string, AssociatedPaymentsList> associated_payments = 2;
...
}
message AssociatedPaymentsList {
repeated AssociatedPayment associated_payments = 1;
}
message AssociatedPayment {
float amount = 1;
string payment_id = 2;
}
This is looking better, @drmaas, thanks!
I'd really appreciate it if you can include some testing for these updates you've added. I'm sure they work, but the tests will prevent future devs (like me 😅) from breaking the new functionality moving forward.
You can probably just tack on a couple maps infixtures/refs.(yaml|json)
and the expected results in fixtures/refs.proto
@jprobinson PTAL - tests added.
@jprobinson added 2 more tests: using a generic ListValue when there the list items have no object properties, and creating a new type when the list values have an inline set of properties.
🌯 ! Thank you for your work, @drmaas!