Skip to content

Conversation

@yordis
Copy link
Contributor

@yordis yordis commented Nov 26, 2025

Signed-off-by: Yordis Prieto yordis.prieto@gmail.com

Add string support to Enum.value/1 function. The reverse mapping infrastructure was already in place, so I think we should be OK to take in.

@yordis yordis force-pushed the yordis/enum-string branch from 97337d6 to 391b120 Compare November 26, 2025 00:47
@yordis yordis marked this pull request as ready for review November 26, 2025 00:48
@whatyouhide
Copy link
Collaborator

What's the reasoning here instead of a Enum.value(Atom.to_string(...))?

@yordis
Copy link
Contributor Author

yordis commented Nov 26, 2025

@whatyouhide I did not understand your question 😭

The motivation here is that we have these atoms coming as a string from the edge and we have to manually map them to atoms, or do things like String.to_existing_atom/1 that I prefer to avoid, just in case.

So I could use MyEnum.value(v) where v is a string instead of an atom or number.

If I am doing something wrong, please let me know.

@whatyouhide
Copy link
Collaborator

Ah I see your point. This looks good to me then. Should we update any docs?

@yordis
Copy link
Contributor Author

yordis commented Nov 26, 2025

Should we update any docs?

@whatyouhide I looked around, currently, I couldn't figure out where to document this, other than in the CHANGELOG so people are aware of it.

Do you have any suggestions?

@whatyouhide
Copy link
Collaborator

CHANGELOG is fine yep!

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis force-pushed the yordis/enum-string branch from 391b120 to 1d06c36 Compare November 28, 2025 18:27
@yordis
Copy link
Contributor Author

yordis commented Nov 28, 2025

@whatyouhide done, I also added an example of a previous PR as well

@whatyouhide whatyouhide merged commit 1605e2b into elixir-protobuf:main Dec 4, 2025
4 checks passed
@whatyouhide
Copy link
Collaborator

Thank you @yordis 🫶

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