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

Add support for oneof custom options #553

Closed
wants to merge 1 commit into from

Conversation

jcready
Copy link
Contributor

@jcready jcready commented Jul 1, 2023

No description provided.

Comment on lines +119 to +121
} else if (OneofDescriptorProto.is(descriptor) && DescriptorProto.is(this.registry.parentOf(descriptor))) {
optionsTypeName = 'google.protobuf.OneofOptions';
} else if (FieldDescriptorProto.is(descriptor) && DescriptorProto.is(this.registry.parentOf(descriptor))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the way MessageType#is behaves (assignable and no excess properties) means that the order of these if statements is extremely important. I spent an hour trying to figure out why the oneof options weren't being populated when I put the OneofDescriptorProto.is(descriptor) check after the FieldDescriptorProto.is(descriptor) check. Well, it's because FieldDescriptorProto.is(oneofDescriptor) && DescriptorProto.is(this.registry.parentOf(oneofDescriptor)) returns true.

One way to work around this is what I've done in this PR is to have the caller just tell us which option type name goes with the descriptor. But I've also realized that we can just use the MESSAGE_TYPE symbol on the descriptor to know which descriptor MessageType it is and therefore which option type name should be used so I'm going to switch to using that.

@jcready
Copy link
Contributor Author

jcready commented Jul 6, 2023

Closed in favor of #562

@jcready jcready closed this Jul 6, 2023
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.

1 participant