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

fix(dynamite): Allow int enums #1319

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Conversation

Leptopoda
Copy link
Member

fixes #1240
replaces: #1242

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Great job!

@Leptopoda
Copy link
Member Author

I'm not sure this is the best way to do it.
I think that enum_value.name should always return a String.
What about introducing a separate attribute called value that is of type T?

@provokateurin
Copy link
Member

I like the idea, please go ahead

@provokateurin
Copy link
Member

I rebased the branch and confirmed it works with the latest specs. If you want I can push that branch if you haven't done it locally already.

@Leptopoda
Copy link
Member Author

I've already rebased and done this:

What about introducing a separate attribute called value that is of type T?

I'll push it in a few minutes just want to make sure it can be easily adapted without any breaking changes.

@Leptopoda Leptopoda force-pushed the fix/dynamite/int-enums-leptopoda branch from 5ed0eb3 to 8701763 Compare December 20, 2023 22:02
@Leptopoda
Copy link
Member Author

I left it on a separate commit for now so we can roll it back easier.
We can squash them before merging.

@provokateurin
Copy link
Member

I'm confused what you did in the second commit. It looks like you reverted a lot of the changes from the first commit?

@Leptopoda
Copy link
Member Author

Leptopoda commented Dec 21, 2023

You are right.
In the second commit we just have the value getter and can use the built_value_generator for the rest of the enum so no need to do it ourselves.

@Leptopoda Leptopoda force-pushed the fix/dynamite/int-enums-leptopoda branch from 8701763 to a0ad441 Compare December 21, 2023 11:36
@Leptopoda Leptopoda enabled auto-merge December 21, 2023 11:40
feat(dynamite_end_to_end_test): Add test for int enums

Co-authored-by: Nikolas Rimikis <[email protected]>
Signed-off-by: jld3103 <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
@Leptopoda Leptopoda force-pushed the fix/dynamite/int-enums-leptopoda branch from a0ad441 to 967c888 Compare December 21, 2023 11:48
@Leptopoda Leptopoda merged commit 119dfe9 into main Dec 21, 2023
8 checks passed
@Leptopoda Leptopoda deleted the fix/dynamite/int-enums-leptopoda branch December 21, 2023 12:11
Leptopoda added a commit that referenced this pull request Dec 21, 2023
@Leptopoda Leptopoda removed this from the Dynamite packages release milestone May 3, 2024
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.

Integer enums are broken
2 participants