Skip to content
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

Using of the correct name for oneof message and options #124

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

biosvs
Copy link
Contributor

@biosvs biosvs commented Aug 21, 2023

Example of the input protos, for which csProto generates uncompilable code:

message I18nVariable {
  oneof one_of_values {
    string opt_one = 1;
    string opt_two = 2;
  }
}

And another example:

message Msg {
  oneof one_of_values {

  Msg.Tags tags = 1;

  message Tags {
    repeated string tags = 1;
  }
}

In the first case, I18nVariable will be used in generated code, while protobuf-go changes name to I18NVariable (n -> N).
In the second case, Msg_Tags will be used, while protobuf-go changes name of the oneof filed to Msg_Tags_ (Msg_Tags also exists - kinda tricky, I know).

This PR fixes generated code.

Once again, I didn't find unit tests for such thing, so I didn't add my either, sorry.

Signed-off-by: Vitalii Levitskii <vitalii@uber.com>
@dylan-bourque
Copy link
Collaborator

dylan-bourque commented Aug 21, 2023

Thanks for the submission.

... didn't find unit tests ...

Yeah. The gotcha is that the test matrix involves generating code for each permutation of (proto2, proto3) X (gogo, google V1, google V2) and I didn't want to have the "main" module depend on all 3 underlying runtimes.

You can run make generate to build protoc-gen-fastmarshal to ./bin and use that to generate code in each of the example/* sub-folders. Ideally, we would also add a new example message in each of those variants for the edge cases you've uncovered.

@dylan-bourque
Copy link
Collaborator

@biosvs can you run make generate test to regenerate the .pb.go files under the example/ folder for each permutation and validate these changes?

@biosvs
Copy link
Contributor Author

biosvs commented Sep 1, 2023

@dylan-bourque sure, made it.

@dylan-bourque dylan-bourque merged commit cb4498b into CrowdStrike:main Sep 1, 2023
5 checks passed
@dylan-bourque
Copy link
Collaborator

v0.25.0 was tagged this morning and includes this change. Let me know if you find any other issues.

Thank you for the contribution.

@biosvs biosvs deleted the fix-oneof-names branch September 1, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants