-
Notifications
You must be signed in to change notification settings - Fork 172
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
Handling of enums #157
Handling of enums #157
Conversation
More detail in improbable-eng#156
4125c0a
to
7353236
Compare
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.
LGTM.
Technically this is not a breaking change as anyone that was relying on the enum
behavior would have problems at runtime :D
Haha brave 😅 Ok, thanks for merging this so quickly. |
Fortune favours the brave... plus we can rely on feedback from the community if anything goes proper bang :D You can dogfood it yourself by upgrading to |
Hi, this change is causing compile errors for our app where we use the enum as a field type, e.g. the mode field in the below
causes
If I use the generated EnvironmentModeMap interface then it will compile |
@mahileeb Could it be that you are importing Otherwise an example repo/gist would be helpful in order for me to help. |
Hmm, the import looks like:
Is that the correct way to import it? Here's gist of the proto schema and the usage: https://gist.github.com/mahileeb/1ce0c625634d10595351c1110711901f |
Any update on this issue? This is the dealbreaker for upgrading to 0.10 and higher. |
There is an alternative plugin that you can use to solve this. |
We're also stuck with 0.9, this change has made protobuf enums unusable for us also. |
More detail in: #156
Changes
Verification
I checked and there seem to be a fair few tests for enum handling, the code changes are backwards compatible so they were not modified. Not sure if there are any additional tests worth writing? I did however have to change some of the casting in the integration test for maps - not sure if the changes makes sense though 🤔