Skip to content

fix: use correct Rust variant name for enum aliases#94

Closed
jstarks wants to merge 1 commit intoinfluxdata:mainfrom
jstarks:aliases
Closed

fix: use correct Rust variant name for enum aliases#94
jstarks wants to merge 1 commit intoinfluxdata:mainfrom
jstarks:aliases

Conversation

@jstarks
Copy link
Copy Markdown

@jstarks jstarks commented Feb 22, 2023

This change fixes code generation of protobuf enums when aliases are present.

This change fixes code generation of protobuf enums when aliases are
present.
@tustvold
Copy link
Copy Markdown
Contributor

Closed by #103

@tustvold tustvold closed this Sep 17, 2023
@jstarks
Copy link
Copy Markdown
Author

jstarks commented Sep 18, 2023

Won't #103 just ignore the aliases in deserialize? This seems incomplete and will lead to deserialization failures.

@tustvold
Copy link
Copy Markdown
Contributor

You can define aliases by assigning the same value to different enum constants. To do this you need to set the allow_alias option to true, otherwise the protocol compiler generates a warning message when aliases are found. Though all alias values are valid during deserialization, the first value is always used when serializing.

That is a valid point, I would be happy to review a PR to address this aspect and add a corresponding test for it

@jstarks
Copy link
Copy Markdown
Author

jstarks commented Sep 18, 2023

This PR addressed addressed that aspect, but it didn't add an explicit test.

Unfortunately, I am having trouble getting the CLA signed, so it might be better to reimplement this (small) change.

@sporkmonger
Copy link
Copy Markdown
Contributor

sporkmonger commented Nov 1, 2023

I can take a look at this gap in a bit since I don't have any limitations with respect to the CLA.

@smalis-msft
Copy link
Copy Markdown

@sporkmonger @tustvold did the follow up needed here happen, or is this still an issue?

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.

4 participants