-
Notifications
You must be signed in to change notification settings - Fork 7
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
update message_options with test_behavior #29
update message_options with test_behavior #29
Conversation
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.
A few questions but LGTM overall! Will defer to someone more familiar with this setup for approval.
protoc -I src -I test/protobuf/protoc/proto --elixir_out=custom_field_options=true:test/protobuf/protoc/proto_gen --plugin=./protoc-gen-elixir test/protobuf/protoc/proto/enum_options.proto | ||
protoc -I src --elixir_out=lib --plugin=./protoc-gen-elixir elixirpb.proto | ||
protoc -I src --elixir_out=lib --plugin=./protoc-gen-elixir brex_elixirpb.proto | ||
protoc -I src -I test/protobuf/protoc/proto/events --elixir_out=lib --plugin=./protoc-gen-elixir brex_events_extensions.proto | ||
protoc -I src -I test/protobuf/protoc/proto/demoscene --elixir_out=custom_field_options=true:lib --plugin=./protoc-gen-elixir demoscene_extensions.proto |
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.
--elixir_out=custom_field_options=true:lib
Is that right? The syntax is.. strange. Why is --elixir_out
different from the ones above? Especially the brex_events` one as that one is closest to our current use case.
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.
The custom_field_options=true
part, I believe is needed to atomize the enums. We could do without that but it would be nice to be able to use (brex.elixirpb.enum).atomize
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.
Yeah, the syntax is weird here, but this looks correct to me
lib/demoscene_extensions.pb.ex
Outdated
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.
Ohh so these extension files actually live in this repo, and are then brought into credit_card
's build system somehow? I've always wondered why my IDE can't resolve them, even though we do appear to have copies of them elsewhere.
|
||
def message_options do | ||
# credo:disable-for-next-line | ||
[%{test_behavior: :block}] |
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'm not sure how this test suite works - is there a way to actually have something receive a message of this type and assert that this value is in the returned options list?
Separately, and just double checking - is this the same return type as the message_options
introduced by the Brex events change? I didn't see a change to the actual protoc
compiler so I'm guessing this whole PR is using the same compilation method, just adding our demoscene_extensions
to that generation?
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'm not sure how this test suite works
Me neither :)
I don't believe this is actually a test suite. My understanding is that the Makefile
has this protoc-gen-elixir step that compiles a plugin that can be used with protoc
. And this step is where some magic happens. It seems to recognize files like brex_events_extensions.pb.ex, and so the important part for us is to add a demoscene_extensions.pb.ex file and then recompile.
And then these test
files are just outputs of running protoc
with the newly compiled plugin on some sample .proto
files. I don't think there's any test suite that verifies the generated output. They might just be manually inspected.
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.
Separately, and just double checking - is this the same return type as the message_options introduced by the Brex events change?
Yes. This is the same that's in the example file in this repo - https://github.com/brexhq/protobuf-elixir/blob/brex-head/test/protobuf/protoc/proto_gen/extension3.pb.ex#L17. It also looks like this in credit_card - https://github.com/brexhq/credit_card/blob/main/protos/lib/protos/data/events/activity_event.pb.ex#L14
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 think you can write a test similar to
protobuf-elixir/test/protobuf/protoc/generator/enum_test.exs
Lines 27 to 57 in 6ad957f
test "generate/2 generates enum type messages with custom options" do | |
ctx = %Context{package: "", custom_field_options?: true} | |
enum_opts = Google.Protobuf.EnumOptions.new() | |
custom_opts = Brex.Elixirpb.EnumOptions.new(atomize: true) | |
opts = | |
Google.Protobuf.EnumOptions.put_extension( | |
enum_opts, | |
Brex.Elixirpb.PbExtension, | |
:enum, | |
custom_opts | |
) | |
desc = %Google.Protobuf.EnumDescriptorProto{ | |
name: "EnumFoo", | |
options: opts, | |
value: [ | |
Google.Protobuf.EnumValueDescriptorProto.new(name: "ENUM_FOO_A", number: 0), | |
Google.Protobuf.EnumValueDescriptorProto.new(name: "ENUM_FOO_B", number: 1) | |
] | |
} | |
msg = Generator.generate(ctx, desc) | |
assert msg =~ "defmodule EnumFoo do\n" | |
assert msg =~ "use Protobuf, custom_field_options?: true, enum: true\n" | |
assert msg =~ "@type t :: integer | :a | :b\n" | |
refute msg =~ "defstruct " | |
assert msg =~ "field :ENUM_FOO_A, 0\n field :ENUM_FOO_B, 1\n" | |
end | |
end |
test "generate/2 output unchanged if custom_field_options? is false" do |
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.
Oooh... nice. I'll look into that. Thanks!
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.
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.
Overall LGTM, but I think we should add a test that verifies the extension is correctly generated and processed
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.
Thanks for adding the test!
Currently, running
brexctl gen proto
with the new test_behavior MessageOption does not update the message_options function like it does for is_event.Following #26 to add a
demoscene_extensions.pb.ex
.Was able to test by doing
protoc -I src -I test/protobuf/protoc/proto/demoscene --elixir_out=custom_field_options=true:lib --plugin=./protoc-gen-elixir demoscene_extensions.proto
And then recompiling
The generated
extension4.pb.ex
file hastest_behavior
in itsmessage_options
.